| Message ID | 20200521182958.163436-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] tipc: block BH before using dst_cache | expand |
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 >> >
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 >
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 > >> >
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 >>>>>
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.
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.
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. > >
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 ?
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 ? >
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
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.
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.
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; }
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(-)