| Message ID | 20220120112115.448077-5-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: A bunch of fixes | expand |
Hi, On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Upon error the ieee802154_xmit_complete() helper is not called. Only > ieee802154_wake_queue() is called manually. We then leak the skb > structure. > > Free the skb structure upon error before returning. > > There is no Fixes tag applying here, many changes have been made on this > area and the issue kind of always existed. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/net/ieee802154/at86rf230.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c > index 7d67f41387f5..0746150f78cf 100644 > --- a/drivers/net/ieee802154/at86rf230.c > +++ b/drivers/net/ieee802154/at86rf230.c > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context) > kfree(ctx); > > ieee802154_wake_queue(lp->hw); > + dev_kfree_skb_any(lp->tx_skb); as I said in other mails there is more broken, we need a: if (lp->is_tx) { ieee802154_wake_queue(lp->hw); dev_kfree_skb_any(lp->tx_skb); lp->is_tx = 0; } in at86rf230_async_error_recover(). Thanks. - Alex
Hi, On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Upon error the ieee802154_xmit_complete() helper is not called. Only > > ieee802154_wake_queue() is called manually. We then leak the skb > > structure. > > > > Free the skb structure upon error before returning. > > > > There is no Fixes tag applying here, many changes have been made on this > > area and the issue kind of always existed. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/net/ieee802154/at86rf230.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c > > index 7d67f41387f5..0746150f78cf 100644 > > --- a/drivers/net/ieee802154/at86rf230.c > > +++ b/drivers/net/ieee802154/at86rf230.c > > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context) > > kfree(ctx); > > > > ieee802154_wake_queue(lp->hw); > > + dev_kfree_skb_any(lp->tx_skb); > > as I said in other mails there is more broken, we need a: > > if (lp->is_tx) { > ieee802154_wake_queue(lp->hw); > dev_kfree_skb_any(lp->tx_skb); > lp->is_tx = 0; > } > Also we should free the skb at first _then_ wake_queue(). - Alex
Hi, On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Upon error the ieee802154_xmit_complete() helper is not called. Only > > ieee802154_wake_queue() is called manually. We then leak the skb > > structure. > > > > Free the skb structure upon error before returning. > > > > There is no Fixes tag applying here, many changes have been made on this > > area and the issue kind of always existed. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/net/ieee802154/at86rf230.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c > > index 7d67f41387f5..0746150f78cf 100644 > > --- a/drivers/net/ieee802154/at86rf230.c > > +++ b/drivers/net/ieee802154/at86rf230.c > > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context) > > kfree(ctx); > > > > ieee802154_wake_queue(lp->hw); > > + dev_kfree_skb_any(lp->tx_skb); > > as I said in other mails there is more broken, we need a: > > if (lp->is_tx) { > ieee802154_wake_queue(lp->hw); > dev_kfree_skb_any(lp->tx_skb); > lp->is_tx = 0; > } > > in at86rf230_async_error_recover(). > s/at86rf230_async_error_recover/at86rf230_async_error_recover_complete/ move the is_tx = 0 out of at86rf230_async_error_recover(). - Alex
Hi, On Sun, 23 Jan 2022 at 17:41, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote: > > > > Hi, > > > > On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Upon error the ieee802154_xmit_complete() helper is not called. Only > > > ieee802154_wake_queue() is called manually. We then leak the skb > > > structure. > > > > > > Free the skb structure upon error before returning. > > > > > > There is no Fixes tag applying here, many changes have been made on this > > > area and the issue kind of always existed. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/net/ieee802154/at86rf230.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c > > > index 7d67f41387f5..0746150f78cf 100644 > > > --- a/drivers/net/ieee802154/at86rf230.c > > > +++ b/drivers/net/ieee802154/at86rf230.c > > > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context) > > > kfree(ctx); > > > > > > ieee802154_wake_queue(lp->hw); > > > + dev_kfree_skb_any(lp->tx_skb); > > > > as I said in other mails there is more broken, we need a: > > > > if (lp->is_tx) { > > ieee802154_wake_queue(lp->hw); > > dev_kfree_skb_any(lp->tx_skb); > > lp->is_tx = 0; > > } > > > > in at86rf230_async_error_recover(). > > > s/at86rf230_async_error_recover/at86rf230_async_error_recover_complete/ > > move the is_tx = 0 out of at86rf230_async_error_recover(). Sorry, still seeing an issue here. We cannot move is_tx = 0 out of at86rf230_async_error_recover() because switching to RX_AACK_ON races with a new interrupt and is_tx is not correct anymore. We need something new like "was_tx" to remember that it was a tx case for the error handling in at86rf230_async_error_recover_complete(). - Alex
Hi Alexander, alex.aring@gmail.com wrote on Sun, 23 Jan 2022 18:14:12 -0500: > Hi, > > On Sun, 23 Jan 2022 at 17:41, Alexander Aring <alex.aring@gmail.com> wrote: > > > > Hi, > > > > On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote: > > > > > > Hi, > > > > > > On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > Upon error the ieee802154_xmit_complete() helper is not called. Only > > > > ieee802154_wake_queue() is called manually. We then leak the skb > > > > structure. > > > > > > > > Free the skb structure upon error before returning. > > > > > > > > There is no Fixes tag applying here, many changes have been made on this > > > > area and the issue kind of always existed. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/net/ieee802154/at86rf230.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c > > > > index 7d67f41387f5..0746150f78cf 100644 > > > > --- a/drivers/net/ieee802154/at86rf230.c > > > > +++ b/drivers/net/ieee802154/at86rf230.c > > > > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context) > > > > kfree(ctx); > > > > > > > > ieee802154_wake_queue(lp->hw); > > > > + dev_kfree_skb_any(lp->tx_skb); > > > > > > as I said in other mails there is more broken, we need a: > > > > > > if (lp->is_tx) { > > > ieee802154_wake_queue(lp->hw); > > > dev_kfree_skb_any(lp->tx_skb); > > > lp->is_tx = 0; > > > } > > > > > > in at86rf230_async_error_recover(). > > > > > s/at86rf230_async_error_recover/at86rf230_async_error_recover_complete/ > > > > move the is_tx = 0 out of at86rf230_async_error_recover(). > > Sorry, still seeing an issue here. > > We cannot move is_tx = 0 out of at86rf230_async_error_recover() > because switching to RX_AACK_ON races with a new interrupt and is_tx > is not correct anymore. We need something new like "was_tx" to > remember that it was a tx case for the error handling in > at86rf230_async_error_recover_complete(). It wasn't easy to catch... I've added a was_tx boolean which is set at the same time is_tx is reset. Then, in the complete handler, if was_tx was set we reset it and run the kfree/wake calls. I believe this should sort it out. Thanks, Miquèl
diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c index 7d67f41387f5..0746150f78cf 100644 --- a/drivers/net/ieee802154/at86rf230.c +++ b/drivers/net/ieee802154/at86rf230.c @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context) kfree(ctx); ieee802154_wake_queue(lp->hw); + dev_kfree_skb_any(lp->tx_skb); } static void
Upon error the ieee802154_xmit_complete() helper is not called. Only ieee802154_wake_queue() is called manually. We then leak the skb structure. Free the skb structure upon error before returning. There is no Fixes tag applying here, many changes have been made on this area and the issue kind of always existed. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/net/ieee802154/at86rf230.c | 1 + 1 file changed, 1 insertion(+)