All of
 help / color / mirror / Atom feed
To: Schspa Shi <>
Subject: Re: [PATCH] 9p: fix crash when transaction killed
Date: Wed, 30 Nov 2022 15:16:17 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

(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
 refcount isn't zero, we can check its tag)
 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.


  parent reply	other threads:[~2022-11-30  6:17 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 [this message]
2022-11-30  8:14       ` Schspa Shi
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:

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

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

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