| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Apr 2023 09:17:36 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Ertman, David M" <david.m.ertman@...el.com>,
"michal.swiatkowski@...ux.intel.com"
<michal.swiatkowski@...ux.intel.com>,
"marcin.szycik@...ux.intel.com" <marcin.szycik@...ux.intel.com>,
"Chmielewski, Pawel" <pawel.chmielewski@...el.com>,
"Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [PATCH net-next 06/12] ice: Add guard rule when creating FDB in
switchdev
> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: piątek, 21 kwietnia 2023 16:23
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Lobakin, Aleksander <aleksander.lobakin@...el.com>; Ertman, David M
> <david.m.ertman@...el.com>; michal.swiatkowski@...ux.intel.com; marcin.szycik@...ux.intel.com; Chmielewski, Pawel
> <pawel.chmielewski@...el.com>; Samudrala, Sridhar <sridhar.samudrala@...el.com>
> Subject: Re: [PATCH net-next 06/12] ice: Add guard rule when creating FDB in switchdev
>
> From: Wojciech Drewek <wojciech.drewek@...el.com>
> Date: Mon, 17 Apr 2023 11:34:06 +0200
>
> > From: Marcin Szycik <marcin.szycik@...el.com>
> >
> > Introduce new "guard" rule upon FDB entry creation.
> >
> > It matches on src_mac, has valid bit unset, allow_pass_l2 set
> > and has a nop action.
>
> [...]
>
> > +static struct ice_rule_query_data *
> > +ice_eswitch_br_guard_rule_create(struct ice_hw *hw, u16 vsi_idx,
> > + const unsigned char *mac)
> > +{
> > + struct ice_adv_rule_info rule_info = { 0 };
> > + struct ice_rule_query_data *rule;
> > + struct ice_adv_lkup_elem *list;
> > + const u16 lkups_cnt = 1;
> > + int err;
>
> You can initialize it with -%ENOMEM right here in order to...
>
> > +
> > + rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> > + if (!rule) {
> > + err = -ENOMEM;
> > + goto err_exit;
> > + }
> > +
> > + list = kcalloc(lkups_cnt, sizeof(*list), GFP_ATOMIC);
> > + if (!list) {
> > + err = -ENOMEM;
> > + goto err_list_alloc;
> > + }
>
> ...make those 2 ifs goto-oneliners :3 As...
>
> > +
> > + list[0].type = ICE_MAC_OFOS;
> > + ether_addr_copy(list[0].h_u.eth_hdr.src_addr, mac);
> > + eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr);
> > +
> > + rule_info.allow_pass_l2 = true;
> > + rule_info.sw_act.vsi_handle = vsi_idx;
> > + rule_info.sw_act.fltr_act = ICE_NOP;
> > + rule_info.priority = 5;
> > +
> > + err = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, rule);
>
> ...it's overwritten here anyway, so it is safe to init it with an error
> value.
Makes sense, thanks.
>
> > + if (err)
> > + goto err_add_rule;
> > +
> > + return rule;
> > +
> > +err_add_rule:
> > + kfree(list);
> > +err_list_alloc:
> > + kfree(rule);
> > +err_exit:
> > + return ERR_PTR(err);
> > +}
> > +
> > static struct ice_esw_br_flow *
> > ice_eswitch_br_flow_create(struct device *dev, struct ice_hw *hw, u16 vsi_idx,
> > int port_type, const unsigned char *mac)
> > {
> > - struct ice_rule_query_data *fwd_rule;
> > + struct ice_rule_query_data *fwd_rule, *guard_rule;
> > struct ice_esw_br_flow *flow;
> > int err;
> >
> > @@ -155,10 +202,22 @@ ice_eswitch_br_flow_create(struct device *dev, struct ice_hw *hw, u16 vsi_idx,
> > goto err_fwd_rule;
> > }
> >
> > + guard_rule = ice_eswitch_br_guard_rule_create(hw, vsi_idx, mac);
> > + if (IS_ERR(guard_rule)) {
> > + err = PTR_ERR(guard_rule);
>
> Aaah ok, that's what you meant in the previous mails. I see now.
> You can either leave it like that or there's an alternative -- pick the
> one that you like the most:
>
> guard_rule = ice_eswitch_...
> err = PTR_ERR(guard_rule);
> if (err) {
> ...
>
I like it, less ptr <-> macros
> > + dev_err(dev, "Failed to create eswitch bridge %sgress guard rule, err: %d\n",
> > + port_type == ICE_ESWITCH_BR_UPLINK_PORT ? "e" : "in",
> > + err);
>
> You still can print it via "%pe" + @guard_rule instead of @err :p (same
> with @fwd_rule above)
>
> > + goto err_guard_rule;
> > + }
> > +
> > flow->fwd_rule = fwd_rule;
> > + flow->guard_rule = guard_rule;
> >
> > return flow;
>
> [...]
>
> > @@ -4624,7 +4628,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
> > */
> > static u16
> > ice_find_recp(struct ice_hw *hw, struct ice_prot_lkup_ext *lkup_exts,
> > - enum ice_sw_tunnel_type tun_type)
> > + struct ice_adv_rule_info *rinfo)
>
> Can be const I think?
Agree
>
> > {
> > bool refresh_required = true;
> > struct ice_sw_recipe *recp;
>
> [...]
>
> > @@ -5075,6 +5082,14 @@ ice_add_sw_recipe(struct ice_hw *hw, struct ice_sw_recipe *rm,
> > set_bit(buf[recps].recipe_indx,
> > (unsigned long *)buf[recps].recipe_bitmap);
> > buf[recps].content.act_ctrl_fwd_priority = rm->priority;
> > +
> > + if (rm->need_pass_l2)
> > + buf[recps].content.act_ctrl |=
> > + ICE_AQ_RECIPE_ACT_NEED_PASS_L2;
> > +
> > + if (rm->allow_pass_l2)
> > + buf[recps].content.act_ctrl |=
> > + ICE_AQ_RECIPE_ACT_ALLOW_PASS_L2;
>
> I don't like these line breaks :s
>
> type_of_content *cont;
> ...
>
> /* As far as I can see, it can be used above as well */
> cont = &buf[recps].content;
>
> if (rm->need_pass_l2)
> cont->act_ctrl |= ICE_AQ_RECIPE_ACT_NEED_PASS_L2;
> if (rm->allow_pass_l2)
> cont->act_ctrl |= ICE_AQ_RECIPE_ACT_ALLOW_PASS_L2;
>
> > recps++;
> > }
> >
>
> [...]
>
> > @@ -6166,6 +6190,11 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> > act |= ICE_SINGLE_ACT_VSI_FORWARDING | ICE_SINGLE_ACT_DROP |
> > ICE_SINGLE_ACT_VALID_BIT;
> > break;
> > + case ICE_NOP:
> > + act |= (rinfo->sw_act.fwd_id.hw_vsi_id <<
> > + ICE_SINGLE_ACT_VSI_ID_S) & ICE_SINGLE_ACT_VSI_ID_M;
>
> `FIELD_PREP(ICE_SINGLE_ACT_VSI_ID_M, rinfo->sw_act.fwd_id.hw_vsi_id)`?
>
> > + act &= ~ICE_SINGLE_ACT_VALID_BIT;
> > + break;
> > default:
> > status = -EIO;
> > goto err_ice_add_adv_rule;
> > @@ -6446,7 +6475,7 @@ ice_rem_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> > return -EIO;
> > }
> >
> > - rid = ice_find_recp(hw, &lkup_exts, rinfo->tun_type);
> > + rid = ice_find_recp(hw, &lkup_exts, rinfo);
> > /* If did not find a recipe that match the existing criteria */
> > if (rid == ICE_MAX_NUM_RECIPES)
> > return -EINVAL;
> > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
> > index c84b56fe84a5..5ecce39cf1f5 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> > @@ -191,6 +191,8 @@ struct ice_adv_rule_info {
> > u16 vlan_type;
> > u16 fltr_rule_id;
> > u32 priority;
> > + u8 need_pass_l2;
> > + u8 allow_pass_l2;
>
> They can be either true or false, nothing else, right? I'd make them
> occupy 1 bit per var then:
Correct
>
> u16 need_pass_l2:1;
> u16 allow_pass_l2:1;
> u16 src_vsi;
>
> +14 free bits for more flags, no holes (stacked with ::src_vsi).
>
> > u16 src_vsi;
> > struct ice_sw_act_ctrl sw_act;
> > struct ice_adv_rule_flags_info flags_info;
> > @@ -254,6 +256,9 @@ struct ice_sw_recipe {
> > */
> > u8 priority;
> >
> > + u8 need_pass_l2;
> > + u8 allow_pass_l2;
>
> (same with bitfields here, just use u8 :1 instead of u16 here to stack
> with ::priority)
>
> > +
> > struct list_head rg_list;
>
> [...]
>
> Thanks,
> Olek
Powered by blists - more mailing lists