Re: [syzbot ci] Re: net: move netdev_compute_master_upper_features to ndo_set_features
From: Hangbin Liu <hidden>
Date: 2026-03-12 14:34:56
Also in:
bridge, lkml, syzbot
Subsystem:
networking drivers, networking [macsec], the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Sabrina Dubroca, Linus Torvalds
On Thu, Mar 12, 2026 at 12:13:52PM +0100, Sabrina Dubroca wrote:
quoted hunk ↗ jump to hunk
quoted
quoted
quoted
This patch calls netdev_change_features() after __netdev_upper_dev_link(), Which trigger a NETDEV_FEAT_CHANGE notify and calls rtmsg_ifinfo_event() to fill the new link info. Maybe the event is a bit early and macsec has data not ready?But this would still mean that there's a mismatch between if_nlmsg_size() and rtnl_fill_ifinfo(), and your patch is only revealing it. I'll send fixes for the stuff I mentioned, no idea if that's what syzbot saw since we don't have a repro.It looks like even the nipa CI is reproducing the issue, i.e.: https://netdev-ctrl.bots.linux.dev/logs/vmksft/net-dbg/results/554921/17-rtnetlink-sh/ more failures here: https://netdev.bots.linux.dev/contest.html?pw-n=0&branch=net-next-2026-03-12--06-00&pw-n=0&pass=0 the fail in mascsec-offload looks quite deterministic, could you please have a look?Ah crap, sorry Hangbin, you were right. macsec_fill_info() returns -EMSGSIZE when the key length is unexpected, and at this point we haven't set it to its proper value yet. Bandaid solution would be:diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index f6cad0746a02..0f7ef7bfbdde 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c@@ -4337,7 +4337,7 @@ static int macsec_fill_info(struct sk_buff *skb, csid = secy->xpn ? MACSEC_CIPHER_ID_GCM_AES_XPN_256 : MACSEC_CIPHER_ID_GCM_AES_256; break; default: - goto nla_put_failure; + return 0; } if (nla_put_sci(skb, IFLA_MACSEC_SCI, secy->sci,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?
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? 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; + /* If h/w offloading is available, propagate to the device */ if (macsec_is_offloaded(macsec)) { const struct macsec_ops *ops;
@@ -4200,7 +4200,7 @@ static int macsec_newlink(struct net_device *dev, ctx.secy = &macsec->secy; err = macsec_offload(ops->mdo_add_secy, &ctx); if (err) - goto del_dev; + goto unlink; macsec->insert_tx_tag = macsec_needs_tx_tag(macsec, ops);
@@ -4209,7 +4209,7 @@ static int macsec_newlink(struct net_device *dev, err = register_macsec_dev(real_dev, dev); if (err < 0) - goto del_dev; + goto unlink; netdev_update_features(dev); netif_stacked_transfer_operstate(real_dev, dev);
@@ -4219,10 +4219,10 @@ static int macsec_newlink(struct net_device *dev, return 0; -del_dev: - macsec_del_dev(macsec); unlink: netdev_upper_dev_unlink(real_dev, dev); +del_dev: + macsec_del_dev(macsec); unregister: unregister_netdevice(dev); return err;
Thanks Hangbin