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
