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 ffff8801cd5f6c58 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(&rds_tcp_conn_lock);
529 list_for_each_entry_safe(tc, _tc, &tmp_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
On Fri, Jan 12, 2018 at 7:30 PM, Sowmini Varadhan
<[email protected]> wrote:
> 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 ffff8801cd5f6c58 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(&rds_tcp_conn_lock);
> 529 list_for_each_entry_safe(tc, _tc, &tmp_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.
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
On Wed, Feb 14, 2018 at 4:21 PM, Sowmini Varadhan
<[email protected]> wrote:
> 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.
On Wed, 2018-02-14 at 18:16 +0100, Dmitry Vyukov wrote:
> On Wed, Feb 14, 2018 at 6:02 PM, Joe Perches <[email protected]> wrote:
> > On Wed, 2018-02-14 at 16:55 +0100, Dmitry Vyukov wrote:
> > > On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan <[email protected]> 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: <[email protected]>
> > > 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
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
On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan
<[email protected]> 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.
>
> 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: <[email protected]>
There probably was some reason to enforce this, so I think I will
change the syzbot email template to include <>.
Thanks!
On Wed, 2018-02-14 at 16:55 +0100, Dmitry Vyukov wrote:
> On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan <[email protected]> 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: <[email protected]>
> 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;
On Wed, Feb 14, 2018 at 6:02 PM, Joe Perches <[email protected]> wrote:
> On Wed, 2018-02-14 at 16:55 +0100, Dmitry Vyukov wrote:
>> On Wed, Feb 14, 2018 at 4:35 PM, Sowmini Varadhan <[email protected]> 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: <[email protected]>
>> 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?
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
On Wed, Feb 14, 2018 at 7:49 PM, Jason Gunthorpe <[email protected]> wrote:
> 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?