Re: [syzbot ci] Re: net: move netdev_compute_master_upper_features to ndo_set_features

2026-03-12, 14:34:44 +0000, Hangbin Liu wrote:
> On Thu, Mar 12, 2026 at 12:13:52PM +0100, Sabrina Dubroca wrote:
> > Proper fix (so that the notification we're sending during
> > upper_dev_link has full linkinfo) would be to move
> > netdev_upper_dev_link() to after macsec_changelink_common() and fix up
> > the error handling. I don't see anything in macsec_add_dev or
> > macsec_changelink_common that needs the device to be linked. But
> 
> If we move the netdev_upper_dev_link() after macsec_changelink_common(),
> we will not goto nla_put_failure via default, right?

Yes.

> > anyway it doesn't make sense for macsec_fill_info to return -EMSGSIZE
> > on invalid data, so the "bandaid" should be included as well.
> > 
> > Should this be part of this series (either just the "bandaid" or the
> > "proper fix"+bandaid), since we never saw a problem before?
> 
> Since macsec need the "bandaid" fix either way. How about you post the
> "bandaid" fix to net. And I include the "proper fix" in this series for
> net-next?

But I don't think it's needed in net. Am I missing a codepath (before
your series) where macsec_fill_info could be called for the new device
before macsec_newlink returns? If not, it doesn't really qualify as a
fix, that's why I was asking Paolo.


> BTW, here is my draft patch, would you like to review it first?
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index f6cad0746a02..1f8da4c291c6 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -4161,10 +4161,6 @@ static int macsec_newlink(struct net_device *dev,
>  	lockdep_set_class(&dev->addr_list_lock,
>  			  &macsec_netdev_addr_lock_key);
>  
> -	err = netdev_upper_dev_link(real_dev, dev, extack);
> -	if (err < 0)
> -		goto unregister;
> -
>  	/* need to be already registered so that ->init has run and
>  	 * the MAC addr is set
>  	 */
> @@ -4177,12 +4173,12 @@ static int macsec_newlink(struct net_device *dev,
>  
>  	if (rx_handler && sci_exists(real_dev, sci)) {
>  		err = -EBUSY;
> -		goto unlink;
> +		goto unregister;
>  	}
>  
>  	err = macsec_add_dev(dev, sci, icv_len);
>  	if (err)
> -		goto unlink;
> +		goto unregister;
>  
>  	if (data) {
>  		err = macsec_changelink_common(dev, data);
> @@ -4190,6 +4186,10 @@ static int macsec_newlink(struct net_device *dev,
>  			goto del_dev;
>  	}
>  
> +	err = netdev_upper_dev_link(real_dev, dev, extack);
> +	if (err < 0)
> +		goto unlink;

This one shouldn't be "goto unlink" since we haven't linked yet. I'm
not sure netdev_upper_dev_unlink can handle "not linked yet" devices.

Otherwise this looks ok.

-- 
Sabrina