Re: KASAN: use-after-free Read in rds_tcp_tune
On Wed, Feb 14, 2018 at 7:49 PM, Jason Gunthorpewrote: > On Wed, Feb 14, 2018 at 10:35:55AM -0500, Sowmini Varadhan wrote: >> On (02/14/18 16:28), Dmitry Vyukov wrote: >> > syzbot is probably not seeing this problem. However if you don't add >> > the Reported-by tag to commit, nor provide syz fix tag, it will >> > consider it as "open". One consequence of this is that it is still on >> > our radars. Another consequence is that syzbot will never report bugs >> > in rds_tcp_tune ever again as it thinks that it's the same known bug, >> > so no point in bothering anybody. >> >> understood, I think I saw this in the original syzbot mail as well, >> but I was hesitant to actually add the tag because the fix was >> based on code-inspection only, and I would have felt more comfortable >> about asserting the Reported-by if I'd done a clear-cut before/after >> verification. > > I think the point is you have to clear it from syzbot to get it to > even test your patches, even if you are not totally sure your patch > fixes it? Sorry, I failed to parse this sentence. Can you please rephrase it?
Re: KASAN: use-after-free Read in rds_tcp_tune
On Wed, Feb 14, 2018 at 10:35:55AM -0500, Sowmini Varadhan wrote: > On (02/14/18 16:28), Dmitry Vyukov wrote: > > syzbot is probably not seeing this problem. However if you don't add > > the Reported-by tag to commit, nor provide syz fix tag, it will > > consider it as "open". One consequence of this is that it is still on > > our radars. Another consequence is that syzbot will never report bugs > > in rds_tcp_tune ever again as it thinks that it's the same known bug, > > so no point in bothering anybody. > > understood, I think I saw this in the original syzbot mail as well, > but I was hesitant to actually add the tag because the fix was > based on code-inspection only, and I would have felt more comfortable > about asserting the Reported-by if I'd done a clear-cut before/after > verification. I think the point is you have to clear it from syzbot to get it to even test your patches, even if you are not totally sure your patch fixes it? Jason
Re: KASAN: use-after-free Read in rds_tcp_tune
On Wed, 2018-02-14 at 18:16 +0100, Dmitry Vyukov wrote: > On Wed, Feb 14, 2018 at 6:02 PM, Joe Percheswrote: > > On Wed, 2018-02-14 at 16:55 +0100, Dmitry Vyukov wrote: > > > On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan > > > wrote: > > > > btw, checkpatch.pl complains about the syzbot*@syzkaller.appspotmail.com > > > > addresses as "Unrecognized email address", we should fix that > > > > error from checkpatch at some point. > > > > > > Interesting. Looking at checkpatch.pl I think it wants all addresses > > > to be in <>, i.e. > > > Reported-by: > > > There probably was some reason to enforce this, so I think I will > > > change the syzbot email template to include <>. > > > Thanks! > > > > Not really. > > > > It's the somewhat unusual + in the address > > that perl needs quoted before a substitution. > > > > I believe this fixes it in checkpatch. > > --- > > scripts/checkpatch.pl | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > > @@ -1075,7 +1075,7 @@ sub parse_email { > > } elsif ($formatted_email =~ /(\S+\@\S+)(.*)$/) { > > $address = $1; > > $comment = $2 if defined $2; > > - $formatted_email =~ s/$address.*$//; > > + $formatted_email =~ s/\Q$address\E.*$//; > > $name = $formatted_email; > > $name = trim($name); > > $name =~ s/^\"|\"$//g; > > > I can confirm that running > $ git show HEAD | scripts/checkpatch.pl - > on a commit that contains a syzbot Reported-by tag does not produce > the warning anymore. > > Joe, do you mind mailing this as patch? 'course not. It's nice you tested it. I did on the last thousand commits without apparent change. $ git log --format=oneline --no-merges -1000 | \ cut -f1 -d" " | \ while read commit ; do \ ./scripts/checkpatch.pl --git $commit --types=bad_sign_off --terse ; \ done
Re: KASAN: use-after-free Read in rds_tcp_tune
On Wed, Feb 14, 2018 at 6:02 PM, Joe Percheswrote: > On Wed, 2018-02-14 at 16:55 +0100, Dmitry Vyukov wrote: >> On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan >> wrote: >> > btw, checkpatch.pl complains about the syzbot*@syzkaller.appspotmail.com >> > addresses as "Unrecognized email address", we should fix that >> > error from checkpatch at some point. >> >> Interesting. Looking at checkpatch.pl I think it wants all addresses >> to be in <>, i.e. >> Reported-by: >> There probably was some reason to enforce this, so I think I will >> change the syzbot email template to include <>. >> Thanks! > > Not really. > > It's the somewhat unusual + in the address > that perl needs quoted before a substitution. > > I believe this fixes it in checkpatch. > --- > scripts/checkpatch.pl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 3d4040322ae1..2b8397da39d3 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1075,7 +1075,7 @@ sub parse_email { > } elsif ($formatted_email =~ /(\S+\@\S+)(.*)$/) { > $address = $1; > $comment = $2 if defined $2; > - $formatted_email =~ s/$address.*$//; > + $formatted_email =~ s/\Q$address\E.*$//; > $name = $formatted_email; > $name = trim($name); > $name =~ s/^\"|\"$//g; I can confirm that running $ git show HEAD | scripts/checkpatch.pl - on a commit that contains a syzbot Reported-by tag does not produce the warning anymore. Joe, do you mind mailing this as patch?
Re: KASAN: use-after-free Read in rds_tcp_tune
On Wed, 2018-02-14 at 16:55 +0100, Dmitry Vyukov wrote: > On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan >wrote: > > btw, checkpatch.pl complains about the syzbot*@syzkaller.appspotmail.com > > addresses as "Unrecognized email address", we should fix that > > error from checkpatch at some point. > > Interesting. Looking at checkpatch.pl I think it wants all addresses > to be in <>, i.e. > Reported-by: > There probably was some reason to enforce this, so I think I will > change the syzbot email template to include <>. > Thanks! Not really. It's the somewhat unusual + in the address that perl needs quoted before a substitution. I believe this fixes it in checkpatch. --- scripts/checkpatch.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 3d4040322ae1..2b8397da39d3 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1075,7 +1075,7 @@ sub parse_email { } elsif ($formatted_email =~ /(\S+\@\S+)(.*)$/) { $address = $1; $comment = $2 if defined $2; - $formatted_email =~ s/$address.*$//; + $formatted_email =~ s/\Q$address\E.*$//; $name = $formatted_email; $name = trim($name); $name =~ s/^\"|\"$//g;
Re: KASAN: use-after-free Read in rds_tcp_tune
On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhanwrote: > On (02/14/18 16:28), Dmitry Vyukov wrote: >> syzbot is probably not seeing this problem. However if you don't add >> the Reported-by tag to commit, nor provide syz fix tag, it will >> consider it as "open". One consequence of this is that it is still on >> our radars. Another consequence is that syzbot will never report bugs >> in rds_tcp_tune ever again as it thinks that it's the same known bug, >> so no point in bothering anybody. > > understood, I think I saw this in the original syzbot mail as well, > but I was hesitant to actually add the tag because the fix was > based on code-inspection only, and I would have felt more comfortable > about asserting the Reported-by if I'd done a clear-cut before/after > verification. > > btw, checkpatch.pl complains about the syzbot*@syzkaller.appspotmail.com > addresses as "Unrecognized email address", we should fix that > error from checkpatch at some point. Interesting. Looking at checkpatch.pl I think it wants all addresses to be in <>, i.e. Reported-by: There probably was some reason to enforce this, so I think I will change the syzbot email template to include <>. Thanks!
Re: KASAN: use-after-free Read in rds_tcp_tune
On (02/14/18 16:28), Dmitry Vyukov wrote: > syzbot is probably not seeing this problem. However if you don't add > the Reported-by tag to commit, nor provide syz fix tag, it will > consider it as "open". One consequence of this is that it is still on > our radars. Another consequence is that syzbot will never report bugs > in rds_tcp_tune ever again as it thinks that it's the same known bug, > so no point in bothering anybody. understood, I think I saw this in the original syzbot mail as well, but I was hesitant to actually add the tag because the fix was based on code-inspection only, and I would have felt more comfortable about asserting the Reported-by if I'd done a clear-cut before/after verification. btw, checkpatch.pl complains about the syzbot*@syzkaller.appspotmail.com addresses as "Unrecognized email address", we should fix that error from checkpatch at some point. --Sowmini
Re: KASAN: use-after-free Read in rds_tcp_tune
On Wed, Feb 14, 2018 at 4:21 PM, Sowmini Varadhanwrote: > On (02/14/18 16:11), Dmitry Vyukov wrote: >> >> Hi Sowmini, >> >> Was this ever fixed? What's the fix? This still hangs as open. Please >> provide "syz fix" tag. > > Are you still seeing this problem? > > I had expected that the changes around rds_destroy_pending - see commit > ebeeb1ad9b8a - would have taken care of this (note that ebeeb1ad9b8a > refactors/updates 3db6e0d172c9) but those fixes were done by inspection > only. In other words, I was never able to reproduce this, so we may > still have missed some race condition. syzbot is probably not seeing this problem. However if you don't add the Reported-by tag to commit, nor provide syz fix tag, it will consider it as "open". One consequence of this is that it is still on our radars. Another consequence is that syzbot will never report bugs in rds_tcp_tune ever again as it thinks that it's the same known bug, so no point in bothering anybody.
Re: KASAN: use-after-free Read in rds_tcp_tune
On (02/14/18 16:11), Dmitry Vyukov wrote: > > Hi Sowmini, > > Was this ever fixed? What's the fix? This still hangs as open. Please > provide "syz fix" tag. Are you still seeing this problem? I had expected that the changes around rds_destroy_pending - see commit ebeeb1ad9b8a - would have taken care of this (note that ebeeb1ad9b8a refactors/updates 3db6e0d172c9) but those fixes were done by inspection only. In other words, I was never able to reproduce this, so we may still have missed some race condition. --Sowmini
Re: KASAN: use-after-free Read in rds_tcp_tune
On Fri, Jan 12, 2018 at 7:30 PM, Sowmini Varadhanwrote: > On (01/11/18 21:29), syzbot wrote: >> == >> BUG: KASAN: use-after-free in rds_tcp_tune+0x491/0x520 net/rds/tcp.c:397 >> Read of size 4 at addr 8801cd5f6c58 by task kworker/u4:4/4954 > > Just had an offline discussion with santosh around this, here's a summary > of that discussion for the archives: > > Looks like an rds_connect_worker workq got scheduled after the > netns was deleted. This could happen if an an rds_connection got > added between lines 528 and 529 of > > 506 static void rds_tcp_kill_sock(struct net *net) > : > /* code to pull out all the rds_connections that should be destroyed */ > : > 528 spin_unlock_irq(_tcp_conn_lock); > 529 list_for_each_entry_safe(tc, _tc, _list, t_tcp_node) > 530 rds_conn_destroy(tc->t_cpath->cp_conn); > > Such an rds_connection would miss out the rds_conn_destroy() > loop (that cancels all pending work) and (if it was scheduled > after netns deletion) could trigger the use-after-free. > > Evaluating various fixes for this (including using _bh instead of _irq > as suggested by santosh), I'll get back with a patch soon. Hi Sowmini, Was this ever fixed? What's the fix? This still hangs as open. Please provide "syz fix" tag.
Re: KASAN: use-after-free Read in rds_tcp_tune
On (01/11/18 21:29), syzbot wrote: > == > BUG: KASAN: use-after-free in rds_tcp_tune+0x491/0x520 net/rds/tcp.c:397 > Read of size 4 at addr 8801cd5f6c58 by task kworker/u4:4/4954 Just had an offline discussion with santosh around this, here's a summary of that discussion for the archives: Looks like an rds_connect_worker workq got scheduled after the netns was deleted. This could happen if an an rds_connection got added between lines 528 and 529 of 506 static void rds_tcp_kill_sock(struct net *net) : /* code to pull out all the rds_connections that should be destroyed */ : 528 spin_unlock_irq(_tcp_conn_lock); 529 list_for_each_entry_safe(tc, _tc, _list, t_tcp_node) 530 rds_conn_destroy(tc->t_cpath->cp_conn); Such an rds_connection would miss out the rds_conn_destroy() loop (that cancels all pending work) and (if it was scheduled after netns deletion) could trigger the use-after-free. Evaluating various fixes for this (including using _bh instead of _irq as suggested by santosh), I'll get back with a patch soon. --Sowmini