Received-SPF: pass (google.com: domain of linux-nfs+bounces-3644-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 10 Jun 2024 22:57:42 -0400
From: Mike Snitzer <snitzer@kernel.org>
To: NeilBrown <neilb@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>, linux-nfs@vger.kernel.org,
	Chuck Lever <chuck.lever@oracle.com>,
	Trond Myklebust <trondmy@hammerspace.com>, snitzer@hammerspace.com
Subject: Re: [for-6.11 PATCH 04/29] sunrpc: handle NULL req->defer in
 cache_defer_req
Message-ID: <Zme9JgfkefRy9tqZ@kernel.org>
References: <>
 <903610ef28512c03fd8db6b4b57e65b3a8bda8a9.camel@kernel.org>
 <171806779662.14261.6943542763312044917@noble.neil.brown.name>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <171806779662.14261.6943542763312044917@noble.neil.brown.name>

Hi Neil,

On Tue, Jun 11, 2024 at 11:03:16AM +1000, NeilBrown wrote:
> On Mon, 10 Jun 2024, Jeff Layton wrote:
> > On Fri, 2024-06-07 at 10:26 -0400, Mike Snitzer wrote:
> > > From: Weston Andros Adamson <dros@primarydata.com>
> > > 
> > > Dont crash with a NULL pointer dereference when req->defer isn't
> > > set. This is needed for the localio path.
> > > 
> > > Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
> > > Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > ---
> > > ?net/sunrpc/cache.c | 2 ++
> > > ?1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> > > index 95ff74706104..b757b891382c 100644
> > > --- a/net/sunrpc/cache.c
> > > +++ b/net/sunrpc/cache.c
> > > @@ -714,6 +714,8 @@ static bool cache_defer_req(struct cache_req
> > > *req, struct cache_head *item)
> > > ?			return false;
> > > ?	}
> > > ?
> > > +	if (!req->defer)
> > > +		return false;
> > > ?	dreq = req->defer(req);
> > > ?	if (dreq == NULL)
> > > ?		return false;
> > 
> > I've gone over it many times, but I still don't quite "get" the
> > deferral handling code. I think the above is probably safe, but please
> > do Cc Neil Brown on later postings of this series since he has a better
> > grasp of that code.
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> 
> The patch is bound to be "safe" in a technical sense, but I wonder why
> it is necessary.  And if we add code that isn't necessary we could make
> the result look confusing, which isn't "safe" in a social sense...
> 
> ->defer is always set non-NULL before svc_process() is called, and I
> don't think cache_defer_req() can be reached without svc_process() being
> called.  So I cannot see how ->defer could possibly be NULL.
> 
> Can you remove this patch and see if you can trigger a crash.  If you
> can I'd love to see the kernel stack.

I removed the patch (and also the patch that exported svc_defer) and
I haven't seen any issues.  So I'll drop those 2 patches.

Thanks,
Mike