Re: [PATCH net] net: dsa: don't dereference NULL extack in dsa_slave_changeupper()

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 23 Aug 2022 10:08:34 +0000 Vladimir Oltean wrote:
> On Mon, Aug 22, 2022 at 06:25:23PM -0700, Jakub Kicinski wrote:
> > On Fri, 19 Aug 2022 20:39:25 +0300 Vladimir Oltean wrote:  
> > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > > index c548b969b083..804a00324c8b 100644
> > > --- a/net/dsa/slave.c
> > > +++ b/net/dsa/slave.c
> > > @@ -2487,7 +2487,7 @@ static int dsa_slave_changeupper(struct net_device *dev,
> > >  			if (!err)
> > >  				dsa_bridge_mtu_normalization(dp);
> > >  			if (err == -EOPNOTSUPP) {
> > > -				if (!extack->_msg)
> > > +				if (extack && !extack->_msg)
> > >  					NL_SET_ERR_MSG_MOD(extack,
> > >  							   "Offloading not supported");  
> > 
> > Other offload paths set the extack prior to the driver call,  
> 
> Example?
> 
> > which has the same effect.  
> 
> No, definitely not the same effect. The difference between (a) setting it
> to "Offloading not supported" before the call to dsa_port_bridge_join()
> and (b) setting it to "Offloading not supported" only if dsa_port_bridge_join()
> returned -EOPNOTSUPP is that drivers don't have to set an extack message
> if they return success, or if they don't implement ds->ops->port_bridge_join.
> The behavior changes for a driver that doesn't set the extack but
> returns 0 if I do that.

Hm, I was pretty sure that's what we did in tc, but maybe it was just
discussed and never done. Let me apply, then.

> > Can't we do the same thing here?
> > Do we care about preserving the extack from another notifier 
> > handler or something? Does not seem like that's the case judging 
> > but the commit under Fixes.  
> 
> Preserving yes, from another notifier handler no.
> 
> DSA suppresses the -EOPNOTSUPP error code from this operation and
> returns 0 to user space, along with a warning note via extack.
> 
> The driver's ds->ops->port_bridge_join() method is given an extack.
> Therefore, if the driver has set the extack to anything, presumably it
> is more specific than what DSA has to say.
> 
> > If it is the case (and hopefully not) we should add a new macro wrapper.
> > Manually twiddling with a field starting with an underscore makes
> > me feel dirty. Perhaps I have been writing too much python lately.  
> 
> Ok, can do later (not "net" patch). Also, if you search for _msg in
> net/dsa/ you'll find more occurrences of accessing it directly.