Message ID | 168426600329.74246.10545150506762914826.stgit@oracle-102.nfsv4bat.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | client-side RPC-with-TLS | expand |
Hi Chuck, On Tue, May 16, 2023 at 3:42 PM Chuck Lever <cel@kernel.org> wrote: > > From: Chuck Lever <chuck.lever@oracle.com> > > I'm about to add a use case that does not want RPC_TASK_NULLCREDS. > Refactor rpc_call_null_helper() so that callers provide NULLCREDS > when they need it. > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/clnt.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > index 4cdb539b5854..2dca0ae489ec 100644 > --- a/net/sunrpc/clnt.c > +++ b/net/sunrpc/clnt.c > @@ -2811,8 +2811,7 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, > .rpc_op_cred = cred, > .callback_ops = ops ?: &rpc_null_ops, > .callback_data = data, > - .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN | > - RPC_TASK_NULLCREDS, > + .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN, > }; > > return rpc_run_task(&task_setup_data); > @@ -2820,7 +2819,8 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, > > struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int flags) > { > - return rpc_call_null_helper(clnt, NULL, cred, flags, NULL, NULL); > + return rpc_call_null_helper(clnt, NULL, cred, flags | RPC_TASK_NULLCREDS, > + NULL, NULL); > } > EXPORT_SYMBOL_GPL(rpc_call_null); I think you missed updating rpc_ping() right below this function as well, I'm unable to mount without the flag. Although I do wonder if it would be easier to slightly rename rpc_call_null_helper(), and then create a new rpc_call_null_helper() that appends the RPC_TASK_NULLCREDS flag. Then we don't need to touch current callers, and your new use case could call the renamed function. What do you think? Anna > > @@ -2920,12 +2920,13 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt, > goto success; > } > > - task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC, > - &rpc_cb_add_xprt_call_ops, data); > + task = rpc_call_null_helper(clnt, xprt, NULL, > + RPC_TASK_ASYNC | RPC_TASK_NULLCREDS, > + &rpc_cb_add_xprt_call_ops, data); > if (IS_ERR(task)) > return PTR_ERR(task); > - > data->xps->xps_nunique_destaddr_xprts++; > + > rpc_put_task(task); > success: > return 1; > @@ -2940,7 +2941,8 @@ static int rpc_clnt_add_xprt_helper(struct rpc_clnt *clnt, > int status = -EADDRINUSE; > > /* Test the connection */ > - task = rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL); > + task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_NULLCREDS, > + NULL, NULL); > if (IS_ERR(task)) > return PTR_ERR(task); > > >
> On May 22, 2023, at 4:35 PM, Anna Schumaker <schumaker.anna@gmail.com> wrote: > > Hi Chuck, > > On Tue, May 16, 2023 at 3:42 PM Chuck Lever <cel@kernel.org> wrote: >> >> From: Chuck Lever <chuck.lever@oracle.com> >> >> I'm about to add a use case that does not want RPC_TASK_NULLCREDS. >> Refactor rpc_call_null_helper() so that callers provide NULLCREDS >> when they need it. >> >> Reviewed-by: Jeff Layton <jlayton@kernel.org> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> net/sunrpc/clnt.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c >> index 4cdb539b5854..2dca0ae489ec 100644 >> --- a/net/sunrpc/clnt.c >> +++ b/net/sunrpc/clnt.c >> @@ -2811,8 +2811,7 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, >> .rpc_op_cred = cred, >> .callback_ops = ops ?: &rpc_null_ops, >> .callback_data = data, >> - .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN | >> - RPC_TASK_NULLCREDS, >> + .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN, >> }; >> >> return rpc_run_task(&task_setup_data); >> @@ -2820,7 +2819,8 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, >> >> struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int flags) >> { >> - return rpc_call_null_helper(clnt, NULL, cred, flags, NULL, NULL); >> + return rpc_call_null_helper(clnt, NULL, cred, flags | RPC_TASK_NULLCREDS, >> + NULL, NULL); >> } >> EXPORT_SYMBOL_GPL(rpc_call_null); > > I think you missed updating rpc_ping() right below this function as > well, I'm unable to mount without the flag. Hrm, I haven't seen that problem. But I can take a look. This patch has been around for more than a year. I'm sure forward porting so many times has left some kruft. > Although I do wonder if it > would be easier to slightly rename rpc_call_null_helper(), and then > create a new rpc_call_null_helper() that appends the > RPC_TASK_NULLCREDS flag. Then we don't need to touch current callers, > and your new use case could call the renamed function. > > What do you think? > Anna > >> >> @@ -2920,12 +2920,13 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt, >> goto success; >> } >> >> - task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC, >> - &rpc_cb_add_xprt_call_ops, data); >> + task = rpc_call_null_helper(clnt, xprt, NULL, >> + RPC_TASK_ASYNC | RPC_TASK_NULLCREDS, >> + &rpc_cb_add_xprt_call_ops, data); >> if (IS_ERR(task)) >> return PTR_ERR(task); >> - >> data->xps->xps_nunique_destaddr_xprts++; >> + >> rpc_put_task(task); >> success: >> return 1; >> @@ -2940,7 +2941,8 @@ static int rpc_clnt_add_xprt_helper(struct rpc_clnt *clnt, >> int status = -EADDRINUSE; >> >> /* Test the connection */ >> - task = rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL); >> + task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_NULLCREDS, >> + NULL, NULL); >> if (IS_ERR(task)) >> return PTR_ERR(task); -- Chuck Lever
> On May 22, 2023, at 4:35 PM, Anna Schumaker <schumaker.anna@gmail.com> wrote: > > Hi Chuck, > > On Tue, May 16, 2023 at 3:42 PM Chuck Lever <cel@kernel.org> wrote: >> >> From: Chuck Lever <chuck.lever@oracle.com> >> >> I'm about to add a use case that does not want RPC_TASK_NULLCREDS. >> Refactor rpc_call_null_helper() so that callers provide NULLCREDS >> when they need it. >> >> Reviewed-by: Jeff Layton <jlayton@kernel.org> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> net/sunrpc/clnt.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c >> index 4cdb539b5854..2dca0ae489ec 100644 >> --- a/net/sunrpc/clnt.c >> +++ b/net/sunrpc/clnt.c >> @@ -2811,8 +2811,7 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, >> .rpc_op_cred = cred, >> .callback_ops = ops ?: &rpc_null_ops, >> .callback_data = data, >> - .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN | >> - RPC_TASK_NULLCREDS, >> + .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN, >> }; >> >> return rpc_run_task(&task_setup_data); >> @@ -2820,7 +2819,8 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, >> >> struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int flags) >> { >> - return rpc_call_null_helper(clnt, NULL, cred, flags, NULL, NULL); >> + return rpc_call_null_helper(clnt, NULL, cred, flags | RPC_TASK_NULLCREDS, >> + NULL, NULL); >> } >> EXPORT_SYMBOL_GPL(rpc_call_null); > > I think you missed updating rpc_ping() right below this function That was on purpose, IIRC. rpc_ping() is supposed to pick up the authentication flavor from the rpc_clnt. In the case of a ping with TLS, it will use RPC_AUTH_TLS. Everyone else should be using NULL creds. What credential are you seeing on the wire for the NULL request? > as well, I'm unable to mount without the flag. Although I do wonder if it > would be easier to slightly rename rpc_call_null_helper(), and then > create a new rpc_call_null_helper() that appends the > RPC_TASK_NULLCREDS flag. Then we don't need to touch current callers, > and your new use case could call the renamed function. > > What do you think? > Anna > >> >> @@ -2920,12 +2920,13 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt, >> goto success; >> } >> >> - task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC, >> - &rpc_cb_add_xprt_call_ops, data); >> + task = rpc_call_null_helper(clnt, xprt, NULL, >> + RPC_TASK_ASYNC | RPC_TASK_NULLCREDS, >> + &rpc_cb_add_xprt_call_ops, data); >> if (IS_ERR(task)) >> return PTR_ERR(task); >> - >> data->xps->xps_nunique_destaddr_xprts++; >> + >> rpc_put_task(task); >> success: >> return 1; >> @@ -2940,7 +2941,8 @@ static int rpc_clnt_add_xprt_helper(struct rpc_clnt *clnt, >> int status = -EADDRINUSE; >> >> /* Test the connection */ >> - task = rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL); >> + task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_NULLCREDS, >> + NULL, NULL); >> if (IS_ERR(task)) >> return PTR_ERR(task); >> >> >> -- Chuck Lever
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c index 4cdb539b5854..2dca0ae489ec 100644 --- a/net/sunrpc/clnt.c +++ b/net/sunrpc/clnt.c @@ -2811,8 +2811,7 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, .rpc_op_cred = cred, .callback_ops = ops ?: &rpc_null_ops, .callback_data = data, - .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN | - RPC_TASK_NULLCREDS, + .flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN, }; return rpc_run_task(&task_setup_data); @@ -2820,7 +2819,8 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt, struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int flags) { - return rpc_call_null_helper(clnt, NULL, cred, flags, NULL, NULL); + return rpc_call_null_helper(clnt, NULL, cred, flags | RPC_TASK_NULLCREDS, + NULL, NULL); } EXPORT_SYMBOL_GPL(rpc_call_null); @@ -2920,12 +2920,13 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt, goto success; } - task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC, - &rpc_cb_add_xprt_call_ops, data); + task = rpc_call_null_helper(clnt, xprt, NULL, + RPC_TASK_ASYNC | RPC_TASK_NULLCREDS, + &rpc_cb_add_xprt_call_ops, data); if (IS_ERR(task)) return PTR_ERR(task); - data->xps->xps_nunique_destaddr_xprts++; + rpc_put_task(task); success: return 1; @@ -2940,7 +2941,8 @@ static int rpc_clnt_add_xprt_helper(struct rpc_clnt *clnt, int status = -EADDRINUSE; /* Test the connection */ - task = rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL); + task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_NULLCREDS, + NULL, NULL); if (IS_ERR(task)) return PTR_ERR(task);