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@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > Signed-off-by: Lance Shelton <lance.shelton@xxxxxxxxxxxxxxx> > > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> > 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
