Re: [for-6.11 PATCH 04/29] sunrpc: handle NULL req->defer in cache_defer_req

[Mike Snitzer <snitzer@kernel.org>]

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