Messages in this thread |  | | Date | Wed, 6 May 2026 19:55:04 +0200 | | From | Sabrina Dubroca <> | | Subject | Re: [PATCH] macsec: defer RX SA cleanup from RCU callback to workqueue |
| |
2026-05-06, 18:01:07 +0800, alexjlzheng@gmail.com wrote: > From: Jinliang Zheng <alexjlzheng@tencent.com> > > crypto_free_aead() can call vunmap() internally (e.g. via > dma_free_attrs() in hardware crypto drivers like hisi_sec2), which > must not be called from softirq context.
Ok.
> free_rxsa() is an RCU callback and therefore runs in softirq context, > causing a kernel crash when the underlying AEAD implementation > performs DMA unmapping during tfm destruction: > > vunmap+0x4c/0x70 > __iommu_dma_free+0xd0/0x138 > dma_free_attrs+0xf4/0x100 > sec_aead_exit+0x64/0xb8 [hisi_sec2] > crypto_destroy_tfm+0x98/0x110 > free_rxsa+0x28/0x50 [macsec] > rcu_do_batch+0x184/0x460 > rcu_core+0xf4/0x1f8 > handle_softirqs+0x118/0x330 > > Fix this by splitting free_rxsa() into two parts: the RCU callback > now only schedules a work item, and the actual resource release > (crypto_free_aead, free_percpu, kfree) is done in a workqueue > handler running in process context. > > Add a destroy_work field to struct macsec_rx_sa and initialize it > in init_rx_sa().
TXSAs go through exactly the same process (destruct via RCU and call crypto_free_aead). I guess they would need exactly the same fix.
> Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
Missing a Fixes tag (most likely c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")).
> drivers/net/macsec.c | 13 +++++++++++-- > include/net/macsec.h | 2 ++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c > index f6cad0746a02..dabd3d2598ae 100644 > --- a/drivers/net/macsec.c > +++ b/drivers/net/macsec.c > @@ -174,15 +174,23 @@ static void macsec_rxsc_put(struct macsec_rx_sc *sc) > call_rcu(&sc->rcu_head, free_rx_sc_rcu); > } > > -static void free_rxsa(struct rcu_head *head) > +static void free_rxsa_work(struct work_struct *work) > { > - struct macsec_rx_sa *sa = container_of(head, struct macsec_rx_sa, rcu); > + struct macsec_rx_sa *sa = container_of(work, struct macsec_rx_sa, > + destroy_work); > > crypto_free_aead(sa->key.tfm); > free_percpu(sa->stats); > kfree(sa); > } > > +static void free_rxsa(struct rcu_head *head) > +{ > + struct macsec_rx_sa *sa = container_of(head, struct macsec_rx_sa, rcu); > + > + schedule_work(&sa->destroy_work); > +}
This is quite ugly. I'd prefer to change the call_rcu() in macsec_rxsa_put() to the schedule_work(), and then add a synchronize_rcu() (to replace the current call_rcu()'s effects) at the start of free_rxsa_work().
In addition, you need to modify macsec_exit() so that it waits on the free_rxsa_work() calls. Otherwise, if they happen after the module has finished unloading, the kernel will crash. Currently there's an rcu_barrier() that waits for free_rxsa() running as RCU callback, but it won't wait for the new work.
-- Sabrina
|  |