* [PATCH net] tipc: block BH before using dst_cache
@ 2020-05-21 18:29 Eric Dumazet
2020-05-22 5:56 ` Xin Long
2020-05-22 22:39 ` David Miller
0 siblings, 2 replies; 13+ messages in thread
From: Eric Dumazet @ 2020-05-21 18:29 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, Xin Long, Jon Maloy, syzbot
dst_cache_get() documents it must be used with BH disabled.
sysbot reported :
BUG: using smp_processor_id() in preemptible [00000000] code: /21697
caller is dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68
CPU: 0 PID: 21697 Comm: Not tainted 5.7.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x188/0x20d lib/dump_stack.c:118
check_preemption_disabled lib/smp_processor_id.c:47 [inline]
debug_smp_processor_id.cold+0x88/0x9b lib/smp_processor_id.c:57
dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68
tipc_udp_xmit.isra.0+0xb9/0xad0 net/tipc/udp_media.c:164
tipc_udp_send_msg+0x3e6/0x490 net/tipc/udp_media.c:244
tipc_bearer_xmit_skb+0x1de/0x3f0 net/tipc/bearer.c:526
tipc_enable_bearer+0xb2f/0xd60 net/tipc/bearer.c:331
__tipc_nl_bearer_enable+0x2bf/0x390 net/tipc/bearer.c:995
tipc_nl_bearer_enable+0x1e/0x30 net/tipc/bearer.c:1003
genl_family_rcv_msg_doit net/netlink/genetlink.c:673 [inline]
genl_family_rcv_msg net/netlink/genetlink.c:718 [inline]
genl_rcv_msg+0x627/0xdf0 net/netlink/genetlink.c:735
netlink_rcv_skb+0x15a/0x410 net/netlink/af_netlink.c:2469
genl_rcv+0x24/0x40 net/netlink/genetlink.c:746
netlink_unicast_kernel net/netlink/af_netlink.c:1303 [inline]
netlink_unicast+0x537/0x740 net/netlink/af_netlink.c:1329
netlink_sendmsg+0x882/0xe10 net/netlink/af_netlink.c:1918
sock_sendmsg_nosec net/socket.c:652 [inline]
sock_sendmsg+0xcf/0x120 net/socket.c:672
____sys_sendmsg+0x6bf/0x7e0 net/socket.c:2362
___sys_sendmsg+0x100/0x170 net/socket.c:2416
__sys_sendmsg+0xec/0x1b0 net/socket.c:2449
do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
entry_SYSCALL_64_after_hwframe+0x49/0xb3
RIP: 0033:0x45ca29
Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media")
Cc: Xin Long <lucien.xin@gmail.com>
Cc: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
net/tipc/udp_media.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index d6620ad535461a4d04ed5ba90569ce8b7df9f994..28a283f26a8dff24d613e6ed57e5e69d894dae66 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -161,9 +161,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb,
struct udp_bearer *ub, struct udp_media_addr *src,
struct udp_media_addr *dst, struct dst_cache *cache)
{
- struct dst_entry *ndst = dst_cache_get(cache);
+ struct dst_entry *ndst;
int ttl, err = 0;
+ local_bh_disable();
+ ndst = dst_cache_get(cache);
if (dst->proto == htons(ETH_P_IP)) {
struct rtable *rt = (struct rtable *)ndst;
@@ -210,9 +212,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb,
src->port, dst->port, false);
#endif
}
+ local_bh_enable();
return err;
tx_error:
+ local_bh_enable();
kfree_skb(skb);
return err;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-21 18:29 [PATCH net] tipc: block BH before using dst_cache Eric Dumazet @ 2020-05-22 5:56 ` Xin Long [not found] ` <CANn89i+x=xbXoKekC6bF_ZMBRMY_mkmuVbNSW3LcRncsiZGd_g@mail.gmail.com> 2020-05-22 22:39 ` David Miller 1 sibling, 1 reply; 13+ messages in thread From: Xin Long @ 2020-05-22 5:56 UTC (permalink / raw) To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Jon Maloy, syzbot On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: > > dst_cache_get() documents it must be used with BH disabled. Interesting, I thought under rcu_read_lock() is enough, which calls preempt_disable(). have you checked other places where dst_cache_get() are used? > > sysbot reported : > > BUG: using smp_processor_id() in preemptible [00000000] code: /21697 > caller is dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 > CPU: 0 PID: 21697 Comm: Not tainted 5.7.0-rc6-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x188/0x20d lib/dump_stack.c:118 > check_preemption_disabled lib/smp_processor_id.c:47 [inline] > debug_smp_processor_id.cold+0x88/0x9b lib/smp_processor_id.c:57 > dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 > tipc_udp_xmit.isra.0+0xb9/0xad0 net/tipc/udp_media.c:164 > tipc_udp_send_msg+0x3e6/0x490 net/tipc/udp_media.c:244 > tipc_bearer_xmit_skb+0x1de/0x3f0 net/tipc/bearer.c:526 > tipc_enable_bearer+0xb2f/0xd60 net/tipc/bearer.c:331 > __tipc_nl_bearer_enable+0x2bf/0x390 net/tipc/bearer.c:995 > tipc_nl_bearer_enable+0x1e/0x30 net/tipc/bearer.c:1003 > genl_family_rcv_msg_doit net/netlink/genetlink.c:673 [inline] > genl_family_rcv_msg net/netlink/genetlink.c:718 [inline] > genl_rcv_msg+0x627/0xdf0 net/netlink/genetlink.c:735 > netlink_rcv_skb+0x15a/0x410 net/netlink/af_netlink.c:2469 > genl_rcv+0x24/0x40 net/netlink/genetlink.c:746 > netlink_unicast_kernel net/netlink/af_netlink.c:1303 [inline] > netlink_unicast+0x537/0x740 net/netlink/af_netlink.c:1329 > netlink_sendmsg+0x882/0xe10 net/netlink/af_netlink.c:1918 > sock_sendmsg_nosec net/socket.c:652 [inline] > sock_sendmsg+0xcf/0x120 net/socket.c:672 > ____sys_sendmsg+0x6bf/0x7e0 net/socket.c:2362 > ___sys_sendmsg+0x100/0x170 net/socket.c:2416 > __sys_sendmsg+0xec/0x1b0 net/socket.c:2449 > do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 > entry_SYSCALL_64_after_hwframe+0x49/0xb3 > RIP: 0033:0x45ca29 > > Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media") > Cc: Xin Long <lucien.xin@gmail.com> > Cc: Jon Maloy <jon.maloy@ericsson.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: syzbot <syzkaller@googlegroups.com> > --- > net/tipc/udp_media.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c > index d6620ad535461a4d04ed5ba90569ce8b7df9f994..28a283f26a8dff24d613e6ed57e5e69d894dae66 100644 > --- a/net/tipc/udp_media.c > +++ b/net/tipc/udp_media.c > @@ -161,9 +161,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, > struct udp_bearer *ub, struct udp_media_addr *src, > struct udp_media_addr *dst, struct dst_cache *cache) > { > - struct dst_entry *ndst = dst_cache_get(cache); > + struct dst_entry *ndst; > int ttl, err = 0; > > + local_bh_disable(); > + ndst = dst_cache_get(cache); > if (dst->proto == htons(ETH_P_IP)) { > struct rtable *rt = (struct rtable *)ndst; > > @@ -210,9 +212,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, > src->port, dst->port, false); > #endif > } > + local_bh_enable(); > return err; > > tx_error: > + local_bh_enable(); > kfree_skb(skb); > return err; > } > -- > 2.27.0.rc0.183.gde8f92d652-goog > ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <CANn89i+x=xbXoKekC6bF_ZMBRMY_mkmuVbNSW3LcRncsiZGd_g@mail.gmail.com>]
* Re: [PATCH net] tipc: block BH before using dst_cache [not found] ` <CANn89i+x=xbXoKekC6bF_ZMBRMY_mkmuVbNSW3LcRncsiZGd_g@mail.gmail.com> @ 2020-05-22 5:55 ` Eric Dumazet 2020-05-22 6:18 ` Xin Long 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2020-05-22 5:55 UTC (permalink / raw) To: Xin Long; +Cc: David S . Miller, netdev, Eric Dumazet, Jon Maloy, syzbot Resend to the list in non HTML form On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: >> >> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: >> > >> > dst_cache_get() documents it must be used with BH disabled. >> Interesting, I thought under rcu_read_lock() is enough, which calls >> preempt_disable(). > > > rcu_read_lock() does not disable BH, never. > > And rcu_read_lock() does not necessarily disable preemption. > > >> >> have you checked other places where dst_cache_get() are used? > > > > Yes, other paths are fine. > >> >> >> > >> > sysbot reported : >> > >> > BUG: using smp_processor_id() in preemptible [00000000] code: /21697 >> > caller is dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 >> > CPU: 0 PID: 21697 Comm: Not tainted 5.7.0-rc6-syzkaller #0 >> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 >> > Call Trace: >> > __dump_stack lib/dump_stack.c:77 [inline] >> > dump_stack+0x188/0x20d lib/dump_stack.c:118 >> > check_preemption_disabled lib/smp_processor_id.c:47 [inline] >> > debug_smp_processor_id.cold+0x88/0x9b lib/smp_processor_id.c:57 >> > dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 >> > tipc_udp_xmit.isra.0+0xb9/0xad0 net/tipc/udp_media.c:164 >> > tipc_udp_send_msg+0x3e6/0x490 net/tipc/udp_media.c:244 >> > tipc_bearer_xmit_skb+0x1de/0x3f0 net/tipc/bearer.c:526 >> > tipc_enable_bearer+0xb2f/0xd60 net/tipc/bearer.c:331 >> > __tipc_nl_bearer_enable+0x2bf/0x390 net/tipc/bearer.c:995 >> > tipc_nl_bearer_enable+0x1e/0x30 net/tipc/bearer.c:1003 >> > genl_family_rcv_msg_doit net/netlink/genetlink.c:673 [inline] >> > genl_family_rcv_msg net/netlink/genetlink.c:718 [inline] >> > genl_rcv_msg+0x627/0xdf0 net/netlink/genetlink.c:735 >> > netlink_rcv_skb+0x15a/0x410 net/netlink/af_netlink.c:2469 >> > genl_rcv+0x24/0x40 net/netlink/genetlink.c:746 >> > netlink_unicast_kernel net/netlink/af_netlink.c:1303 [inline] >> > netlink_unicast+0x537/0x740 net/netlink/af_netlink.c:1329 >> > netlink_sendmsg+0x882/0xe10 net/netlink/af_netlink.c:1918 >> > sock_sendmsg_nosec net/socket.c:652 [inline] >> > sock_sendmsg+0xcf/0x120 net/socket.c:672 >> > ____sys_sendmsg+0x6bf/0x7e0 net/socket.c:2362 >> > ___sys_sendmsg+0x100/0x170 net/socket.c:2416 >> > __sys_sendmsg+0xec/0x1b0 net/socket.c:2449 >> > do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 >> > entry_SYSCALL_64_after_hwframe+0x49/0xb3 >> > RIP: 0033:0x45ca29 >> > >> > Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media") >> > Cc: Xin Long <lucien.xin@gmail.com> >> > Cc: Jon Maloy <jon.maloy@ericsson.com> >> > Signed-off-by: Eric Dumazet <edumazet@google.com> >> > Reported-by: syzbot <syzkaller@googlegroups.com> >> > --- >> > net/tipc/udp_media.c | 6 +++++- >> > 1 file changed, 5 insertions(+), 1 deletion(-) >> > >> > diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c >> > index d6620ad535461a4d04ed5ba90569ce8b7df9f994..28a283f26a8dff24d613e6ed57e5e69d894dae66 100644 >> > --- a/net/tipc/udp_media.c >> > +++ b/net/tipc/udp_media.c >> > @@ -161,9 +161,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, >> > struct udp_bearer *ub, struct udp_media_addr *src, >> > struct udp_media_addr *dst, struct dst_cache *cache) >> > { >> > - struct dst_entry *ndst = dst_cache_get(cache); >> > + struct dst_entry *ndst; >> > int ttl, err = 0; >> > >> > + local_bh_disable(); >> > + ndst = dst_cache_get(cache); >> > if (dst->proto == htons(ETH_P_IP)) { >> > struct rtable *rt = (struct rtable *)ndst; >> > >> > @@ -210,9 +212,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, >> > src->port, dst->port, false); >> > #endif >> > } >> > + local_bh_enable(); >> > return err; >> > >> > tx_error: >> > + local_bh_enable(); >> > kfree_skb(skb); >> > return err; >> > } >> > -- >> > 2.27.0.rc0.183.gde8f92d652-goog >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 5:55 ` Eric Dumazet @ 2020-05-22 6:18 ` Xin Long 2020-05-22 15:01 ` Jon Maloy 2020-05-22 15:52 ` Eric Dumazet 0 siblings, 2 replies; 13+ messages in thread From: Xin Long @ 2020-05-22 6:18 UTC (permalink / raw) To: Eric Dumazet Cc: David S . Miller, netdev, Eric Dumazet, syzbot, tipc-discussion, jmaloy On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: > > Resend to the list in non HTML form > > > On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: > >> > >> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: > >> > > >> > dst_cache_get() documents it must be used with BH disabled. > >> Interesting, I thought under rcu_read_lock() is enough, which calls > >> preempt_disable(). > > > > > > rcu_read_lock() does not disable BH, never. > > > > And rcu_read_lock() does not necessarily disable preemption. Then I need to think again if it's really worth using dst_cache here. Also add tipc-discussion and Jon to CC list. Thanks. > > > > > >> > >> have you checked other places where dst_cache_get() are used? > > > > > > > > Yes, other paths are fine. > > > >> > >> > >> > > >> > sysbot reported : > >> > > >> > BUG: using smp_processor_id() in preemptible [00000000] code: /21697 > >> > caller is dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 > >> > CPU: 0 PID: 21697 Comm: Not tainted 5.7.0-rc6-syzkaller #0 > >> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > >> > Call Trace: > >> > __dump_stack lib/dump_stack.c:77 [inline] > >> > dump_stack+0x188/0x20d lib/dump_stack.c:118 > >> > check_preemption_disabled lib/smp_processor_id.c:47 [inline] > >> > debug_smp_processor_id.cold+0x88/0x9b lib/smp_processor_id.c:57 > >> > dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 > >> > tipc_udp_xmit.isra.0+0xb9/0xad0 net/tipc/udp_media.c:164 > >> > tipc_udp_send_msg+0x3e6/0x490 net/tipc/udp_media.c:244 > >> > tipc_bearer_xmit_skb+0x1de/0x3f0 net/tipc/bearer.c:526 > >> > tipc_enable_bearer+0xb2f/0xd60 net/tipc/bearer.c:331 > >> > __tipc_nl_bearer_enable+0x2bf/0x390 net/tipc/bearer.c:995 > >> > tipc_nl_bearer_enable+0x1e/0x30 net/tipc/bearer.c:1003 > >> > genl_family_rcv_msg_doit net/netlink/genetlink.c:673 [inline] > >> > genl_family_rcv_msg net/netlink/genetlink.c:718 [inline] > >> > genl_rcv_msg+0x627/0xdf0 net/netlink/genetlink.c:735 > >> > netlink_rcv_skb+0x15a/0x410 net/netlink/af_netlink.c:2469 > >> > genl_rcv+0x24/0x40 net/netlink/genetlink.c:746 > >> > netlink_unicast_kernel net/netlink/af_netlink.c:1303 [inline] > >> > netlink_unicast+0x537/0x740 net/netlink/af_netlink.c:1329 > >> > netlink_sendmsg+0x882/0xe10 net/netlink/af_netlink.c:1918 > >> > sock_sendmsg_nosec net/socket.c:652 [inline] > >> > sock_sendmsg+0xcf/0x120 net/socket.c:672 > >> > ____sys_sendmsg+0x6bf/0x7e0 net/socket.c:2362 > >> > ___sys_sendmsg+0x100/0x170 net/socket.c:2416 > >> > __sys_sendmsg+0xec/0x1b0 net/socket.c:2449 > >> > do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 > >> > entry_SYSCALL_64_after_hwframe+0x49/0xb3 > >> > RIP: 0033:0x45ca29 > >> > > >> > Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media") > >> > Cc: Xin Long <lucien.xin@gmail.com> > >> > Cc: Jon Maloy <jon.maloy@ericsson.com> > >> > Signed-off-by: Eric Dumazet <edumazet@google.com> > >> > Reported-by: syzbot <syzkaller@googlegroups.com> > >> > --- > >> > net/tipc/udp_media.c | 6 +++++- > >> > 1 file changed, 5 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c > >> > index d6620ad535461a4d04ed5ba90569ce8b7df9f994..28a283f26a8dff24d613e6ed57e5e69d894dae66 100644 > >> > --- a/net/tipc/udp_media.c > >> > +++ b/net/tipc/udp_media.c > >> > @@ -161,9 +161,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, > >> > struct udp_bearer *ub, struct udp_media_addr *src, > >> > struct udp_media_addr *dst, struct dst_cache *cache) > >> > { > >> > - struct dst_entry *ndst = dst_cache_get(cache); > >> > + struct dst_entry *ndst; > >> > int ttl, err = 0; > >> > > >> > + local_bh_disable(); > >> > + ndst = dst_cache_get(cache); > >> > if (dst->proto == htons(ETH_P_IP)) { > >> > struct rtable *rt = (struct rtable *)ndst; > >> > > >> > @@ -210,9 +212,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, > >> > src->port, dst->port, false); > >> > #endif > >> > } > >> > + local_bh_enable(); > >> > return err; > >> > > >> > tx_error: > >> > + local_bh_enable(); > >> > kfree_skb(skb); > >> > return err; > >> > } > >> > -- > >> > 2.27.0.rc0.183.gde8f92d652-goog > >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 6:18 ` Xin Long @ 2020-05-22 15:01 ` Jon Maloy 2020-05-22 15:57 ` Eric Dumazet 2020-05-22 15:52 ` Eric Dumazet 1 sibling, 1 reply; 13+ messages in thread From: Jon Maloy @ 2020-05-22 15:01 UTC (permalink / raw) To: Xin Long, Eric Dumazet Cc: David S . Miller, netdev, Eric Dumazet, syzbot, tipc-discussion On 5/22/20 2:18 AM, Xin Long wrote: > On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: >> Resend to the list in non HTML form >> >> >> On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: >>> >>> >>> On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: >>>> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: >>>>> dst_cache_get() documents it must be used with BH disabled. >>>> Interesting, I thought under rcu_read_lock() is enough, which calls >>>> preempt_disable(). >>> >>> rcu_read_lock() does not disable BH, never. >>> >>> And rcu_read_lock() does not necessarily disable preemption. > Then I need to think again if it's really worth using dst_cache here. > > Also add tipc-discussion and Jon to CC list. The suggested solution will affect all bearers, not only UDP, so it is not a good. Is there anything preventing us from disabling preemtion inside the scope of the rcu lock? ///jon > > Thanks. > >>> >>>> have you checked other places where dst_cache_get() are used? >>> >>> >>> Yes, other paths are fine. >>> >>>> >>>>> sysbot reported : >>>>> >>>>> BUG: using smp_processor_id() in preemptible [00000000] code: /21697 >>>>> caller is dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 >>>>> CPU: 0 PID: 21697 Comm: Not tainted 5.7.0-rc6-syzkaller #0 >>>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 >>>>> Call Trace: >>>>> __dump_stack lib/dump_stack.c:77 [inline] >>>>> dump_stack+0x188/0x20d lib/dump_stack.c:118 >>>>> check_preemption_disabled lib/smp_processor_id.c:47 [inline] >>>>> debug_smp_processor_id.cold+0x88/0x9b lib/smp_processor_id.c:57 >>>>> dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 >>>>> tipc_udp_xmit.isra.0+0xb9/0xad0 net/tipc/udp_media.c:164 >>>>> tipc_udp_send_msg+0x3e6/0x490 net/tipc/udp_media.c:244 >>>>> tipc_bearer_xmit_skb+0x1de/0x3f0 net/tipc/bearer.c:526 >>>>> tipc_enable_bearer+0xb2f/0xd60 net/tipc/bearer.c:331 >>>>> __tipc_nl_bearer_enable+0x2bf/0x390 net/tipc/bearer.c:995 >>>>> tipc_nl_bearer_enable+0x1e/0x30 net/tipc/bearer.c:1003 >>>>> genl_family_rcv_msg_doit net/netlink/genetlink.c:673 [inline] >>>>> genl_family_rcv_msg net/netlink/genetlink.c:718 [inline] >>>>> genl_rcv_msg+0x627/0xdf0 net/netlink/genetlink.c:735 >>>>> netlink_rcv_skb+0x15a/0x410 net/netlink/af_netlink.c:2469 >>>>> genl_rcv+0x24/0x40 net/netlink/genetlink.c:746 >>>>> netlink_unicast_kernel net/netlink/af_netlink.c:1303 [inline] >>>>> netlink_unicast+0x537/0x740 net/netlink/af_netlink.c:1329 >>>>> netlink_sendmsg+0x882/0xe10 net/netlink/af_netlink.c:1918 >>>>> sock_sendmsg_nosec net/socket.c:652 [inline] >>>>> sock_sendmsg+0xcf/0x120 net/socket.c:672 >>>>> ____sys_sendmsg+0x6bf/0x7e0 net/socket.c:2362 >>>>> ___sys_sendmsg+0x100/0x170 net/socket.c:2416 >>>>> __sys_sendmsg+0xec/0x1b0 net/socket.c:2449 >>>>> do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 >>>>> entry_SYSCALL_64_after_hwframe+0x49/0xb3 >>>>> RIP: 0033:0x45ca29 >>>>> >>>>> Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media") >>>>> Cc: Xin Long <lucien.xin@gmail.com> >>>>> Cc: Jon Maloy <jon.maloy@ericsson.com> >>>>> Signed-off-by: Eric Dumazet <edumazet@google.com> >>>>> Reported-by: syzbot <syzkaller@googlegroups.com> >>>>> --- >>>>> net/tipc/udp_media.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c >>>>> index d6620ad535461a4d04ed5ba90569ce8b7df9f994..28a283f26a8dff24d613e6ed57e5e69d894dae66 100644 >>>>> --- a/net/tipc/udp_media.c >>>>> +++ b/net/tipc/udp_media.c >>>>> @@ -161,9 +161,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, >>>>> struct udp_bearer *ub, struct udp_media_addr *src, >>>>> struct udp_media_addr *dst, struct dst_cache *cache) >>>>> { >>>>> - struct dst_entry *ndst = dst_cache_get(cache); >>>>> + struct dst_entry *ndst; >>>>> int ttl, err = 0; >>>>> >>>>> + local_bh_disable(); >>>>> + ndst = dst_cache_get(cache); >>>>> if (dst->proto == htons(ETH_P_IP)) { >>>>> struct rtable *rt = (struct rtable *)ndst; >>>>> >>>>> @@ -210,9 +212,11 @@ static int tipc_udp_xmit(struct net *net, struct sk_buff *skb, >>>>> src->port, dst->port, false); >>>>> #endif >>>>> } >>>>> + local_bh_enable(); >>>>> return err; >>>>> >>>>> tx_error: >>>>> + local_bh_enable(); >>>>> kfree_skb(skb); >>>>> return err; >>>>> } >>>>> -- >>>>> 2.27.0.rc0.183.gde8f92d652-goog >>>>> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 15:01 ` Jon Maloy @ 2020-05-22 15:57 ` Eric Dumazet 2020-05-22 19:47 ` Jon Maloy 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2020-05-22 15:57 UTC (permalink / raw) To: Jon Maloy, Xin Long, Eric Dumazet Cc: David S . Miller, netdev, Eric Dumazet, syzbot, tipc-discussion On 5/22/20 8:01 AM, Jon Maloy wrote: > > > On 5/22/20 2:18 AM, Xin Long wrote: >> On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: >>> Resend to the list in non HTML form >>> >>> >>> On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: >>>> >>>> >>>> On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: >>>>> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: >>>>>> dst_cache_get() documents it must be used with BH disabled. >>>>> Interesting, I thought under rcu_read_lock() is enough, which calls >>>>> preempt_disable(). >>>> >>>> rcu_read_lock() does not disable BH, never. >>>> >>>> And rcu_read_lock() does not necessarily disable preemption. >> Then I need to think again if it's really worth using dst_cache here. >> >> Also add tipc-discussion and Jon to CC list. > The suggested solution will affect all bearers, not only UDP, so it is not a good. > Is there anything preventing us from disabling preemtion inside the scope of the rcu lock? > > ///jon > BH is disabled any way few nano seconds later, disabling it a bit earlier wont make any difference. Also, if you intend to make dst_cache BH reentrant, you will have to make that for net-next, not net tree. Please carefully read include/net/dst_cache.h It is very clear about BH requirements. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 15:57 ` Eric Dumazet @ 2020-05-22 19:47 ` Jon Maloy 2020-05-22 20:10 ` Eric Dumazet 0 siblings, 1 reply; 13+ messages in thread From: Jon Maloy @ 2020-05-22 19:47 UTC (permalink / raw) To: Eric Dumazet, Xin Long, Eric Dumazet Cc: David S . Miller, netdev, syzbot, tipc-discussion On 5/22/20 11:57 AM, Eric Dumazet wrote: > > On 5/22/20 8:01 AM, Jon Maloy wrote: >> >> On 5/22/20 2:18 AM, Xin Long wrote: >>> On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: >>>> Resend to the list in non HTML form >>>> >>>> >>>> On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: >>>>> >>>>> On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: >>>>>> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: >>>>>>> dst_cache_get() documents it must be used with BH disabled. >>>>>> Interesting, I thought under rcu_read_lock() is enough, which calls >>>>>> preempt_disable(). >>>>> rcu_read_lock() does not disable BH, never. >>>>> >>>>> And rcu_read_lock() does not necessarily disable preemption. >>> Then I need to think again if it's really worth using dst_cache here. >>> >>> Also add tipc-discussion and Jon to CC list. >> The suggested solution will affect all bearers, not only UDP, so it is not a good. >> Is there anything preventing us from disabling preemtion inside the scope of the rcu lock? >> >> ///jon >> > BH is disabled any way few nano seconds later, disabling it a bit earlier wont make any difference. The point is that if we only disable inside tipc_udp_xmit() (the function pointer call) the change will only affect the UDP bearer, where dst_cache is used. The corresponding calls for the Ethernet and Infiniband bearers don't use dst_cache, and don't need this disabling. So it does makes a difference. ///jon > > Also, if you intend to make dst_cache BH reentrant, you will have to make that for net-next, not net tree. > > Please carefully read include/net/dst_cache.h > > It is very clear about BH requirements. > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 19:47 ` Jon Maloy @ 2020-05-22 20:10 ` Eric Dumazet 2020-05-22 21:37 ` Jon Maloy 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2020-05-22 20:10 UTC (permalink / raw) To: Jon Maloy, Xin Long, Eric Dumazet Cc: David S . Miller, netdev, syzbot, tipc-discussion On 5/22/20 12:47 PM, Jon Maloy wrote: > > > On 5/22/20 11:57 AM, Eric Dumazet wrote: >> >> On 5/22/20 8:01 AM, Jon Maloy wrote: >>> >>> On 5/22/20 2:18 AM, Xin Long wrote: >>>> On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: >>>>> Resend to the list in non HTML form >>>>> >>>>> >>>>> On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: >>>>>> >>>>>> On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: >>>>>>> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: >>>>>>>> dst_cache_get() documents it must be used with BH disabled. >>>>>>> Interesting, I thought under rcu_read_lock() is enough, which calls >>>>>>> preempt_disable(). >>>>>> rcu_read_lock() does not disable BH, never. >>>>>> >>>>>> And rcu_read_lock() does not necessarily disable preemption. >>>> Then I need to think again if it's really worth using dst_cache here. >>>> >>>> Also add tipc-discussion and Jon to CC list. >>> The suggested solution will affect all bearers, not only UDP, so it is not a good. >>> Is there anything preventing us from disabling preemtion inside the scope of the rcu lock? >>> >>> ///jon >>> >> BH is disabled any way few nano seconds later, disabling it a bit earlier wont make any difference. > The point is that if we only disable inside tipc_udp_xmit() (the function pointer call) the change will only affect the UDP bearer, where dst_cache is used. > The corresponding calls for the Ethernet and Infiniband bearers don't use dst_cache, and don't need this disabling. So it does makes a difference. > I honestly do not understand your concern, this makes no sense to me. I have disabled BH _right_ before the dst_cache_get(cache) call, so has no effect if the dst_cache is not used, this should be obvious. If some other paths do not use dst)cache, how can my patch have any effect on them ? What alternative are you suggesting ? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 20:10 ` Eric Dumazet @ 2020-05-22 21:37 ` Jon Maloy 2020-05-22 21:44 ` Eric Dumazet 0 siblings, 1 reply; 13+ messages in thread From: Jon Maloy @ 2020-05-22 21:37 UTC (permalink / raw) To: Eric Dumazet, Xin Long, Eric Dumazet Cc: David S . Miller, netdev, syzbot, tipc-discussion On 5/22/20 4:10 PM, Eric Dumazet wrote: > > On 5/22/20 12:47 PM, Jon Maloy wrote: >> >> On 5/22/20 11:57 AM, Eric Dumazet wrote: >>> On 5/22/20 8:01 AM, Jon Maloy wrote: >>>> On 5/22/20 2:18 AM, Xin Long wrote: >>>>> On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: >>>>>> Resend to the list in non HTML form >>>>>> >>>>>> >>>>>> On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: >>>>>>> On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: >>>>>>>> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: >>>>>>>>> dst_cache_get() documents it must be used with BH disabled. >>>>>>>> Interesting, I thought under rcu_read_lock() is enough, which calls >>>>>>>> preempt_disable(). >>>>>>> rcu_read_lock() does not disable BH, never. >>>>>>> >>>>>>> And rcu_read_lock() does not necessarily disable preemption. >>>>> Then I need to think again if it's really worth using dst_cache here. >>>>> >>>>> Also add tipc-discussion and Jon to CC list. >>>> The suggested solution will affect all bearers, not only UDP, so it is not a good. >>>> Is there anything preventing us from disabling preemtion inside the scope of the rcu lock? >>>> >>>> ///jon >>>> >>> BH is disabled any way few nano seconds later, disabling it a bit earlier wont make any difference. >> The point is that if we only disable inside tipc_udp_xmit() (the function pointer call) the change will only affect the UDP bearer, where dst_cache is used. >> The corresponding calls for the Ethernet and Infiniband bearers don't use dst_cache, and don't need this disabling. So it does makes a difference. >> > I honestly do not understand your concern, this makes no sense to me. > > I have disabled BH _right_ before the dst_cache_get(cache) call, so has no effect if the dst_cache is not used, this should be obvious. Forget my comment. I thought we were discussing to Tetsuo Handa's original patch, and missed that you had posted your own. I have no problems with this one. ///jon > > If some other paths do not use dst)cache, how can my patch have any effect on them ? > > What alternative are you suggesting ? > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 21:37 ` Jon Maloy @ 2020-05-22 21:44 ` Eric Dumazet 2020-05-23 0:28 ` Tetsuo Handa 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2020-05-22 21:44 UTC (permalink / raw) To: Jon Maloy, Tetsuo Handa Cc: Eric Dumazet, Xin Long, David S . Miller, netdev, syzbot, tipc-discussion On Fri, May 22, 2020 at 2:37 PM Jon Maloy <jmaloy@redhat.com> wrote: > > > > On 5/22/20 4:10 PM, Eric Dumazet wrote: > > > > On 5/22/20 12:47 PM, Jon Maloy wrote: > >> > >> On 5/22/20 11:57 AM, Eric Dumazet wrote: > >>> On 5/22/20 8:01 AM, Jon Maloy wrote: > >>>> On 5/22/20 2:18 AM, Xin Long wrote: > >>>>> On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: > >>>>>> Resend to the list in non HTML form > >>>>>> > >>>>>> > >>>>>> On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: > >>>>>>> On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: > >>>>>>>> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: > >>>>>>>>> dst_cache_get() documents it must be used with BH disabled. > >>>>>>>> Interesting, I thought under rcu_read_lock() is enough, which calls > >>>>>>>> preempt_disable(). > >>>>>>> rcu_read_lock() does not disable BH, never. > >>>>>>> > >>>>>>> And rcu_read_lock() does not necessarily disable preemption. > >>>>> Then I need to think again if it's really worth using dst_cache here. > >>>>> > >>>>> Also add tipc-discussion and Jon to CC list. > >>>> The suggested solution will affect all bearers, not only UDP, so it is not a good. > >>>> Is there anything preventing us from disabling preemtion inside the scope of the rcu lock? > >>>> > >>>> ///jon > >>>> > >>> BH is disabled any way few nano seconds later, disabling it a bit earlier wont make any difference. > >> The point is that if we only disable inside tipc_udp_xmit() (the function pointer call) the change will only affect the UDP bearer, where dst_cache is used. > >> The corresponding calls for the Ethernet and Infiniband bearers don't use dst_cache, and don't need this disabling. So it does makes a difference. > >> > > I honestly do not understand your concern, this makes no sense to me. > > > > I have disabled BH _right_ before the dst_cache_get(cache) call, so has no effect if the dst_cache is not used, this should be obvious. > Forget my comment. I thought we were discussing to Tetsuo Handa's > original patch, and missed that you had posted your own. > I have no problems with this one. > Ah, this now makes sense, I was not aware of Tetsuo patch. You are absolutely right, Tetsuo Handa's patch is wrong. Thanks ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 21:44 ` Eric Dumazet @ 2020-05-23 0:28 ` Tetsuo Handa 0 siblings, 0 replies; 13+ messages in thread From: Tetsuo Handa @ 2020-05-23 0:28 UTC (permalink / raw) To: Eric Dumazet, Jon Maloy Cc: Eric Dumazet, Xin Long, David S . Miller, netdev, syzbot, tipc-discussion On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: > dst_cache_get() documents it must be used with BH disabled. Since the report was complaining about preemption at this_cpu_ptr(), and "#syz test" request with my preemption-disable patch no longer complained, I didn't realize that it is documented in dst_cache.h that dst_cache_get() must be called with BH disabled. It is bug-prone that we don't have a check for complaining that BH is not disabled. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-22 6:18 ` Xin Long 2020-05-22 15:01 ` Jon Maloy @ 2020-05-22 15:52 ` Eric Dumazet 1 sibling, 0 replies; 13+ messages in thread From: Eric Dumazet @ 2020-05-22 15:52 UTC (permalink / raw) To: Xin Long, Eric Dumazet Cc: David S . Miller, netdev, Eric Dumazet, syzbot, tipc-discussion, jmaloy On 5/21/20 11:18 PM, Xin Long wrote: > On Fri, May 22, 2020 at 1:55 PM Eric Dumazet <edumazet@google.com> wrote: >> >> Resend to the list in non HTML form >> >> >> On Thu, May 21, 2020 at 10:53 PM Eric Dumazet <edumazet@google.com> wrote: >>> >>> >>> >>> On Thu, May 21, 2020 at 10:50 PM Xin Long <lucien.xin@gmail.com> wrote: >>>> >>>> On Fri, May 22, 2020 at 2:30 AM Eric Dumazet <edumazet@google.com> wrote: >>>>> >>>>> dst_cache_get() documents it must be used with BH disabled. >>>> Interesting, I thought under rcu_read_lock() is enough, which calls >>>> preempt_disable(). >>> >>> >>> rcu_read_lock() does not disable BH, never. >>> >>> And rcu_read_lock() does not necessarily disable preemption. > Then I need to think again if it's really worth using dst_cache here. > > Also add tipc-discussion and Jon to CC list. > > Thanks. What improvements you got with your patch ? Disabling BH a bit earlier wont make any difference. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] tipc: block BH before using dst_cache 2020-05-21 18:29 [PATCH net] tipc: block BH before using dst_cache Eric Dumazet 2020-05-22 5:56 ` Xin Long @ 2020-05-22 22:39 ` David Miller 1 sibling, 0 replies; 13+ messages in thread From: David Miller @ 2020-05-22 22:39 UTC (permalink / raw) To: edumazet; +Cc: netdev, eric.dumazet, lucien.xin, jon.maloy, syzkaller From: Eric Dumazet <edumazet@google.com> Date: Thu, 21 May 2020 11:29:58 -0700 > dst_cache_get() documents it must be used with BH disabled. > > sysbot reported : > > BUG: using smp_processor_id() in preemptible [00000000] code: /21697 > caller is dst_cache_get+0x3a/0xb0 net/core/dst_cache.c:68 ... > Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: syzbot <syzkaller@googlegroups.com> Applied and queued up for -stable, thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-05-23 0:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21 18:29 [PATCH net] tipc: block BH before using dst_cache Eric Dumazet
2020-05-22 5:56 ` Xin Long
[not found] ` <CANn89i+x=xbXoKekC6bF_ZMBRMY_mkmuVbNSW3LcRncsiZGd_g@mail.gmail.com>
2020-05-22 5:55 ` Eric Dumazet
2020-05-22 6:18 ` Xin Long
2020-05-22 15:01 ` Jon Maloy
2020-05-22 15:57 ` Eric Dumazet
2020-05-22 19:47 ` Jon Maloy
2020-05-22 20:10 ` Eric Dumazet
2020-05-22 21:37 ` Jon Maloy
2020-05-22 21:44 ` Eric Dumazet
2020-05-23 0:28 ` Tetsuo Handa
2020-05-22 15:52 ` Eric Dumazet
2020-05-22 22:39 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).