From c78ebe1df01f4ef3fb07be1359bc34df6708d99c Mon Sep 17 00:00:00 2001 From: Sabrina Dubroca Date: Fri, 29 Jul 2016 15:37:53 +0200 Subject: [PATCH 1/3] macsec: fix reference counting on RXSC in macsec_handle_frame Currently, we lookup the RXSC without taking a reference on it. The RXSA holds a reference on the RXSC, but the SA and SC could still both disappear before we take a reference on the SA. Take a reference on the RXSC in macsec_handle_frame. Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") Signed-off-by: Sabrina Dubroca Signed-off-by: David S. Miller --- drivers/net/macsec.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 2d0beb1b801c..718cf98023ff 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -863,6 +863,7 @@ static void macsec_decrypt_done(struct crypto_async_request *base, int err) struct net_device *dev = skb->dev; struct macsec_dev *macsec = macsec_priv(dev); struct macsec_rx_sa *rx_sa = macsec_skb_cb(skb)->rx_sa; + struct macsec_rx_sc *rx_sc = rx_sa->sc; int len, ret; u32 pn; @@ -891,6 +892,7 @@ static void macsec_decrypt_done(struct crypto_async_request *base, int err) out: macsec_rxsa_put(rx_sa); + macsec_rxsc_put(rx_sc); dev_put(dev); } @@ -1106,6 +1108,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb) list_for_each_entry_rcu(macsec, &rxd->secys, secys) { struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci); + sc = sc ? macsec_rxsc_get(sc) : NULL; if (sc) { secy = &macsec->secy; @@ -1180,8 +1183,10 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb) if (IS_ERR(skb)) { /* the decrypt callback needs the reference */ - if (PTR_ERR(skb) != -EINPROGRESS) + if (PTR_ERR(skb) != -EINPROGRESS) { macsec_rxsa_put(rx_sa); + macsec_rxsc_put(rx_sc); + } rcu_read_unlock(); *pskb = NULL; return RX_HANDLER_CONSUMED; @@ -1197,6 +1202,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb) if (rx_sa) macsec_rxsa_put(rx_sa); + macsec_rxsc_put(rx_sc); ret = gro_cells_receive(&macsec->gro_cells, skb); if (ret == NET_RX_SUCCESS) @@ -1212,6 +1218,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb) drop: macsec_rxsa_put(rx_sa); drop_nosa: + macsec_rxsc_put(rx_sc); rcu_read_unlock(); drop_direct: kfree_skb(skb); From 36b232c880c99fc03e135198c7c08d3d4b4f83ab Mon Sep 17 00:00:00 2001 From: Sabrina Dubroca Date: Fri, 29 Jul 2016 15:37:54 +0200 Subject: [PATCH 2/3] macsec: RXSAs don't need to hold a reference on RXSCs Following the previous patch, RXSCs are held and properly refcounted in the RX path (instead of being implicitly held by their SA), so the SA doesn't need to hold a reference on its parent RXSC. This also avoids panics on module unload caused by the double layer of RCU callbacks (call_rcu frees the RXSA, which puts the final reference on the RXSC and allows to free it in its own call_rcu) that commit b196c22af5c3 ("macsec: add rcu_barrier() on module exit") didn't protect against. There were also some refcounting bugs in macsec_add_rxsa where I didn't put the reference on the RXSC on the error paths, which would lead to memory leaks. Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") Signed-off-by: Sabrina Dubroca Signed-off-by: David S. Miller --- drivers/net/macsec.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 718cf98023ff..7f5c5e0f9cf9 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -344,7 +344,6 @@ static void free_rxsa(struct rcu_head *head) crypto_free_aead(sa->key.tfm); free_percpu(sa->stats); - macsec_rxsc_put(sa->sc); kfree(sa); } @@ -1653,7 +1652,7 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) rtnl_lock(); rx_sc = get_rxsc_from_nl(genl_info_net(info), attrs, tb_rxsc, &dev, &secy); - if (IS_ERR(rx_sc) || !macsec_rxsc_get(rx_sc)) { + if (IS_ERR(rx_sc)) { rtnl_unlock(); return PTR_ERR(rx_sc); } From 0759e552bce7257db542e203a01a9ef8843c751e Mon Sep 17 00:00:00 2001 From: Sabrina Dubroca Date: Fri, 29 Jul 2016 15:37:55 +0200 Subject: [PATCH 3/3] macsec: fix negative refcnt on parent link When creation of a macsec device fails because an identical device already exists on this link, the current code decrements the refcnt on the parent link (in ->destructor for the macsec device), but it had not been incremented yet. Move the dev_hold(parent_link) call earlier during macsec device creation. Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") Signed-off-by: Sabrina Dubroca Signed-off-by: David S. Miller --- drivers/net/macsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 7f5c5e0f9cf9..d13e6e15d7b5 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -3179,6 +3179,8 @@ static int macsec_newlink(struct net *net, struct net_device *dev, if (err < 0) return err; + dev_hold(real_dev); + /* need to be already registered so that ->init has run and * the MAC addr is set */ @@ -3207,8 +3209,6 @@ static int macsec_newlink(struct net *net, struct net_device *dev, macsec_generation++; - dev_hold(real_dev); - return 0; del_dev: