[PATCH net-next v2 2/4] macsec: move netdev_upper_dev_link() after macsec_changelink_common()

[Hangbin Liu]

After calling netdev_change_features() in __netdev_upper_dev_link(),
the call stack looks like:

- netdev_upper_dev_link
  - __netdev_upper_dev_link
    - netdev_change_features
      - notifier_call_chain
        - rtnetlink_event
          - rtmsg_ifinfo_event
            - rtmsg_ifinfo_build_skb

In macsec, we call netdev_upper_dev_link() before
macsec_changelink_common(), which causes the fields of the MACsec
Security Entity to be uninitialized. Later, macsec_fill_info() returns
-EMSGSIZE and triggers WARN_ON() in rtmsg_ifinfo_build_skb.

Fix this by moving netdev_upper_dev_link() after
macsec_changelink_common(), and return 0 if secy->key_len is not
initialized yet.

Suggested-by: Sabrina Dubroca <s...@queasysnail.net>
Signed-off-by: Hangbin Liu <liuhang...@gmail.com>
---
 drivers/net/macsec.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index f6cad0746a02..6bb38084bc1e 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 del_dev;
+
        /* 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;
@@ -4337,7 +4337,8 @@ 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;
+               WARN_ON_ONCE(1);
+               return 0;
        }
 
        if (nla_put_sci(skb, IFLA_MACSEC_SCI, secy->sci,

-- 
Git-155)