Received-SPF: pass (google.com: domain of linux-nfs+bounces-3642-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Jeff Layton" <jlayton@kernel.org>
Cc: "Mike Snitzer" <snitzer@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
In-reply-to: <903610ef28512c03fd8db6b4b57e65b3a8bda8a9.camel@kernel.org>
References: <>, <903610ef28512c03fd8db6b4b57e65b3a8bda8a9.camel@kernel.org>
Date: Tue, 11 Jun 2024 11:03:16 +1000
Message-id: <171806779662.14261.6943542763312044917@noble.neil.brown.name>

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.

NeilBrown