| Message ID | 20231218084252.7644-1-zhangyiqun@phytium.com.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xfrm: Use spin_lock_bh() in xfrm_input() | expand |
On Mon, Dec 18, 2023 at 9:43 AM Zhang Yiqun <zhangyiqun@phytium.com.cn> wrote: > > This patch is to change spin_lock() into spin_lock_bh(), which can > disable bottem half in calling. If we leave this as spin_lock(), > it may stuck in a deadlock, because the callback in bottem half in > crypto driver will also call xfrm_input() again. > > Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn> When was the bug added ? We need a FIxes: tag. Also a stack trace to show the deadlock (or lockdep complaint ) would be needed as well. > --- > net/xfrm/xfrm_input.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c > index bd4ce21d76d7..f4cd46d73b1e 100644 > --- a/net/xfrm/xfrm_input.c > +++ b/net/xfrm/xfrm_input.c > @@ -581,7 +581,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) > } > > lock: > - spin_lock(&x->lock); > + spin_lock_bh(&x->lock); > > if (unlikely(x->km.state != XFRM_STATE_VALID)) { > if (x->km.state == XFRM_STATE_ACQ) > @@ -607,7 +607,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) > goto drop_unlock; > } > > - spin_unlock(&x->lock); > + spin_unlock_bh(&x->lock); > > if (xfrm_tunnel_check(skb, x, family)) { > XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMODEERROR); This patch is not correct anyway. There are five places in xfrm_input() where x->lock is either locked or unlocked. Please tell us how this was tested. Thanks.
>diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index >bd4ce21d76d7..f4cd46d73b1e 100644 >--- a/net/xfrm/xfrm_input.c >+++ b/net/xfrm/xfrm_input.c >@@ -581,7 +581,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, >__be32 spi, int encap_type) > } > > lock: >- spin_lock(&x->lock); >+ spin_lock_bh(&x->lock); [Suman] Hi Zhang, There is a spin_lock() after label "resume". Don't we need to change it there? >
On Mon, Dec 18, 2023 at 12:07:50PM +0100, Eric Dumazet wrote: > On Mon, Dec 18, 2023 at 9:43 AM Zhang Yiqun <zhangyiqun@phytium.com.cn> wrote: > > > > This patch is to change spin_lock() into spin_lock_bh(), which can > > disable bottem half in calling. If we leave this as spin_lock(), > > it may stuck in a deadlock, because the callback in bottem half in > > crypto driver will also call xfrm_input() again. > > > > Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn> > > When was the bug added ? > We need a FIxes: tag. This looks more like a 'crypto driver' bug. xfrm_input() runs in the RX path and therefore expects to run with BHs off.
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index bd4ce21d76d7..f4cd46d73b1e 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -581,7 +581,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) } lock: - spin_lock(&x->lock); + spin_lock_bh(&x->lock); if (unlikely(x->km.state != XFRM_STATE_VALID)) { if (x->km.state == XFRM_STATE_ACQ) @@ -607,7 +607,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) goto drop_unlock; } - spin_unlock(&x->lock); + spin_unlock_bh(&x->lock); if (xfrm_tunnel_check(skb, x, family)) { XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMODEERROR);
This patch is to change spin_lock() into spin_lock_bh(), which can disable bottem half in calling. If we leave this as spin_lock(), it may stuck in a deadlock, because the callback in bottem half in crypto driver will also call xfrm_input() again. Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn> --- net/xfrm/xfrm_input.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)