| Message ID | 356fb42c-9cf1-45cd-9233-ac845c507fb7@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-XXX] SUNRPC: Add an IS_ERR() check back to where it was | expand |
Obviously net-XXX is not what I intended. This applies to the nfs tree I think. regards, dan carpenter
On Wed, Oct 11, 2023 at 11:00:22AM +0300, Dan Carpenter wrote: > This IS_ERR() check was deleted during in a cleanup because, at the time, > the rpcb_call_async() function could not return an error pointer. That > changed in commit 25cf32ad5dba ("SUNRPC: Handle allocation failure in > rpc_new_task()") and now it can return an error pointer. Put the check > back. > > A related revert was done in commit 13bd90141804 ("Revert "SUNRPC: > Remove unreachable error condition""). > > Fixes: 037e910b52b0 ("SUNRPC: Remove unreachable error condition in rpcb_getport_async()") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Thanks, I've reviewed the logic of this commit along with the description and it matches up in my mind. Reviewed-by: Simon Horman <horms@kernel.org>
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index 5988a5c5ff3f..102c3818bc54 100644 --- a/net/sunrpc/rpcb_clnt.c +++ b/net/sunrpc/rpcb_clnt.c @@ -769,6 +769,10 @@ void rpcb_getport_async(struct rpc_task *task) child = rpcb_call_async(rpcb_clnt, map, proc); rpc_release_client(rpcb_clnt); + if (IS_ERR(child)) { + /* rpcb_map_release() has freed the arguments */ + return; + } xprt->stat.bind_count++; rpc_put_task(child);
This IS_ERR() check was deleted during in a cleanup because, at the time, the rpcb_call_async() function could not return an error pointer. That changed in commit 25cf32ad5dba ("SUNRPC: Handle allocation failure in rpc_new_task()") and now it can return an error pointer. Put the check back. A related revert was done in commit 13bd90141804 ("Revert "SUNRPC: Remove unreachable error condition""). Fixes: 037e910b52b0 ("SUNRPC: Remove unreachable error condition in rpcb_getport_async()") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- It's possible Smatch was responsible for generating the original warning because it warns for unnecessary NULL checks. But generally, there was a future error pointer implied. Those warnings are just a hint, not a command. net/sunrpc/rpcb_clnt.c | 4 ++++ 1 file changed, 4 insertions(+)