| 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
| ||
|
Message-ID: <MW4PR11MB577640D359484E99EB333D05FD659@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Wed, 26 Apr 2023 09:50:56 +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: Drewek, Wojciech
> Sent: wtorek, 25 kwietnia 2023 11:18
> To: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; 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
>
>
>
> > -----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.
> >
> > [...]
> >
[...]
> >
> > > + 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
Actually it won't work, PTR_ERR would not convert pointer to 0 in case of success.
>
> > > + 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