* [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
[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-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
* 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 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-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-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
* 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
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).