| Message ID | 20240607142646.20924-5-snitzer@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs/nfsd: add support for localio bypass | expand |
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.
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
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
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;