netdev - RE: [PATCH net-next 05/12] ice: Switchdev FDB events support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Apr 2023 08:45:08 +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 05/12] ice: Switchdev FDB events support



> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: czwartek, 20 kwietnia 2023 19:00
> To: Drewek, Wojciech <wojciech.drewek@...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 05/12] ice: Switchdev FDB events support
> 
> From: Wojciech Drewek <wojciech.drewek@...el.com>
> Date: Thu, 20 Apr 2023 13:27:11 +0200
> 
> >
> >
> >> -----Original Message-----
> >> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> >> Sent: środa, 19 kwietnia 2023 17:39
> >> 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 05/12] ice: Switchdev FDB events support
> 
> [...]
> 
> >> (no types shorter than u32 on the stack reminder)
> >>
> >>> +			       const unsigned char *mac)
> >>> +{
> >>> +	struct ice_adv_rule_info rule_info = { 0 };
> >>> +	struct ice_rule_query_data *rule;
> >>> +	struct ice_adv_lkup_elem *list;
> >>> +	u16 lkups_cnt = 1;
> >>
> >> Why have it as variable if it doesn't change? Just embed it into the
> >> ice_add_adv_rule() call and replace kcalloc() with kzalloc().
> >
> > It will be useful later, with vlans support lkups_cnt will be equal to 1 or 2.
> > Can we keep it as it is?
> 
> Ah, okay, then it's surely better to keep as-is. Maybe I'd only mention
> then in the commit message that this variable will be expanded to have
> several values later. So that other reviewers won't trigger on the same
> stuff.

Sure thing

> 
> >
> >>
> >>> +	int err;
> >>> +
> >>> +	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> >>> +	if (!rule)
> >>> +		return ERR_PTR(-ENOMEM);
> >>> +
> >>> +	list = kcalloc(lkups_cnt, sizeof(*list), GFP_ATOMIC);
> >>
> >> [...]
> >>
> >>> +	fwd_rule = ice_eswitch_br_fwd_rule_create(hw, vsi_idx, port_type, mac);
> >>> +	if (IS_ERR(fwd_rule)) {
> >>> +		err = PTR_ERR(fwd_rule);
> >>> +		dev_err(dev, "Failed to create eswitch bridge %sgress forward rule, err: %d\n",
> >>> +			port_type == ICE_ESWITCH_BR_UPLINK_PORT ? "e" : "in",
> >>> +			err);
> >>> +		goto err_fwd_rule;
> >>
> >> A bit suboptimal. To print errno pointer, you have %pe modifier, so you
> >> can just print err as:
> >>
> >> 		... forward rule, err: %pe\n", ... : "in", fwd_rule);
> >>
> >> Then you don't need @err at all and then below...
> >
> > This is really cool, but I think it won't work here. I need to keep err in order to
> > return it in the err flow. I can't use fwd_rule for this purpose because
> > return type is ice_esw_br_flow not ice_rule_query_data.
> 
> My bad, forgot to mention. If you want to return error pointer of a type
> different from the return value's one, there's ERR_CAST(). It casts
> error pointer to `void *`, so that there'll be no warnings then.
> Here's nice example: [0]

Another cool thing I've learn, still I don't we can use it here.
In the next patch, another rule is created in  this function, called
guard rule. Its creation can also fail and we have second pointer for it
called (guard_rule).

> 
> >
> >>
> >>> +	}
> >>> +
> >>> +	flow->fwd_rule = fwd_rule;
> >>> +
> >>> +	return flow;
> >>> +
> >>> +err_fwd_rule:
> >>> +	kfree(flow);
> >>> +
> >>> +	return ERR_PTR(err);
> >>
> >> ...you can return @fwd_rule directly.
> >>
> >
> > I can't return @fwd_rule here because return type is different
> > This function is meant to return @flow.
> 
> [...]
> 
> >>> +	struct net_device *dev;
> >>> +	struct ice_esw_br_port *br_port;
> >>> +	struct ice_esw_br_flow *flow;
> >>> +};
> >> [...]
> >>
> >> Thanks,
> >> Olek
> 
> [0]
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-fractional-divider.c#L293
> 
> Thanks,
> Olek