All of lore.kernel.org
 help / color / mirror / Atom feed
From: Schspa Shi <schspa@gmail.com>
To: asmadeus@codewreck.org
Cc: ericvh@gmail.com, lucho@ionkov.net, linux_oss@crudebyte.com,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, v9fs-developer@lists.sourceforge.net,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	syzbot+8f1060e2aaf8ca55220b@syzkaller.appspotmail.com
Subject: Re: [PATCH] 9p: fix crash when transaction killed
Date: Wed, 30 Nov 2022 16:14:32 +0800	[thread overview]
Message-ID: <m2o7sowzas.fsf@gmail.com> (raw)
In-Reply-To: <Y4b1MQaEsPRK+3lF@codewreck.org>


asmadeus@codewreck.org writes:

> (fixed Christophe's address, hopefully that will do for good...)
>
> Schspa Shi wrote on Wed, Nov 30, 2022 at 10:22:44AM +0800:
>> > I'm happy to believe we have a race somewhere (even if no sane server
>> > would produce it), but right now I don't see it looking at the code.. :/
>> 
>> And I think there is a race too. because the syzbot report about 9p fs
>> memory corruption multi times.
>
> Yes, no point in denying that :)
>
>> As for the problem, the p9_tag_lookup only takes the rcu_read_lock when
>> accessing the IDR, why it doesn't take the p9_client->lock? Maybe the
>> root cause is that a lock is missing here.
>
> It shouldn't need to, but happy to try adding it.
> For the logic:
>  - idr_find is RCU-safe (trusting the comment above it)
>  - reqs are alloced in a kmem_cache created with SLAB_TYPESAFE_BY_RCU.
>  This means that if we get a req from idr_find, even if it has just been
>  freed, it either is still in the state it was freed at (hence refcount
>  0, we ignore it) or is another req coming from the same cache (if

If the req was newly alloced(It was at a new page), refcount maybe not
0, there will be problem in this case. It seems we can't relay on this.

We need to set the refcount to zero before add it to idr in p9_tag_alloc.

>  refcount isn't zero, we can check its tag)

As for the release case, the next request will have the same tag with
high probability. It's better to make the tag value to be an increase
sequence, thus will avoid very much possible req reuse.

>  The refcount itself is an atomic operation so doesn't require lock.
>  ... And in the off chance I hadn't considered that we're already
>  dealing with a new request with the same tag here, we'll be updating
>  its status so another receive for it shouldn't use it?...
>
> I don't think adding the client lock helps with anything here, but it'll
> certainly simplify this logic as we then are guaranteed not to get
> obsolete results from idr_find.
>
> Unfortunately adding a lock will slow things down regardless of
> correctness, so it might just make the race much harder to hit without
> fixing it and we might not notice that, so it'd be good to understand
> the race.


-- 
BRs
Schspa Shi

  reply	other threads:[~2022-11-30  9:46 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-29 16:22 [PATCH] 9p: fix crash when transaction killed Schspa Shi
2022-11-29 16:26 ` Schspa Shi
2022-11-29 18:23   ` Christian Schoenebeck
2022-11-29 22:38 ` asmadeus
2022-11-30  2:22   ` Schspa Shi
2022-11-30  3:26     ` Schspa Shi
2022-11-30  6:16     ` asmadeus
2022-11-30  8:14       ` Schspa Shi [this message]
2022-11-30 11:06         ` asmadeus
2022-11-30 12:43           ` Christian Schoenebeck
2022-11-30 12:54             ` asmadeus
2022-11-30 13:25               ` Christian Schoenebeck
2022-11-30 13:40                 ` asmadeus
2022-11-30 13:15           ` Schspa Shi
2022-11-30 13:34             ` asmadeus
2022-12-01  2:26               ` Schspa Shi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m2o7sowzas.fsf@gmail.com \
    --to=schspa@gmail.com \
    --cc=asmadeus@codewreck.org \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=ericvh@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux_oss@crudebyte.com \
    --cc=lucho@ionkov.net \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=syzbot+8f1060e2aaf8ca55220b@syzkaller.appspotmail.com \
    --cc=v9fs-developer@lists.sourceforge.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.