| Message ID | 20211018123812.71482-1-trbecker@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sunrpc: bug on rpc_task_set_client when no client is present. | expand |
On Mon, 2021-10-18 at 09:38 -0300, Thiago Rafael Becker wrote: > If we pass a NULL client to rpc_task_set_client and no client is > attached to the task, then the kernel will crash later. Antecipate > the > crash by checking if a client is available for the task. > > Signed-off-by: Thiago Rafael Becker <trbecker@gmail.com> > --- > net/sunrpc/clnt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > index f056ff931444..ccbc9a9715da 100644 > --- a/net/sunrpc/clnt.c > +++ b/net/sunrpc/clnt.c > @@ -1076,7 +1076,7 @@ void rpc_task_set_transport(struct rpc_task > *task, struct rpc_clnt *clnt) > static > void rpc_task_set_client(struct rpc_task *task, struct rpc_clnt > *clnt) > { > - > + BUG_ON(clnt == NULL && task->tk_client == NULL); > if (clnt != NULL) { > rpc_task_set_transport(task, clnt); > task->tk_client = clnt; I'm not seeing the point of this BUG_ON(). Why not just change this code to not check for clnt == NULL, and let the thing Oops when it tries to dereference clnt?
On Wed, Oct 20, 2021 at 07:04:35PM +0000, Trond Myklebust wrote: Hello, > I'm not seeing the point of this BUG_ON(). Why not just change this > code to not check for clnt == NULL, and let the thing Oops when it > tries to dereference clnt? This was changed in 58f9612c6ea85, prior to that, this was not tested. I'm not sure why this test exists, the only reason I can imagine is to keep the previous task's rpc_client in case the new client is NULL. Decided to go conservative on this, and BUG_ON() when no client is available. Inside the linux source, I don't see how this may happen unless the code has a bug, so I think it's possible to remove this test. -- Thiago > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c index f056ff931444..ccbc9a9715da 100644 --- a/net/sunrpc/clnt.c +++ b/net/sunrpc/clnt.c @@ -1076,7 +1076,7 @@ void rpc_task_set_transport(struct rpc_task *task, struct rpc_clnt *clnt) static void rpc_task_set_client(struct rpc_task *task, struct rpc_clnt *clnt) { - + BUG_ON(clnt == NULL && task->tk_client == NULL); if (clnt != NULL) { rpc_task_set_transport(task, clnt); task->tk_client = clnt;
If we pass a NULL client to rpc_task_set_client and no client is attached to the task, then the kernel will crash later. Antecipate the crash by checking if a client is available for the task. Signed-off-by: Thiago Rafael Becker <trbecker@gmail.com> --- net/sunrpc/clnt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)