* [PATCH] nfsd4: handle failure to find backchannel
@ 2014-07-11 21:16 J. Bruce Fields
2014-07-12 14:02 ` Kinglong Mee
0 siblings, 1 reply; 6+ messages in thread
From: J. Bruce Fields @ 2014-07-11 21:16 UTC (permalink / raw)
To: linux-nfs
The local variable "ses" will be left NULL here in the case we fail to
find a connection. Spotted by a coverity scan.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 2c73cae9899d..fe22cd5c42d3 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
}
spin_unlock(&clp->cl_lock);
+ if (!c)
+ goto out_no_connection;
err = setup_callback_client(clp, &conn, ses);
- if (err) {
- nfsd4_mark_cb_down(clp, err);
- return;
- }
+ if (err)
+ goto out_no_connection;
/* Yay, the callback channel's back! Restart any callbacks: */
list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
run_nfsd4_cb(cb);
+ return;
+out_no_connection:
+ nfsd4_mark_cb_down(clp, err);
+ return;
}
static void nfsd4_do_callback_rpc(struct work_struct *w)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] nfsd4: handle failure to find backchannel 2014-07-11 21:16 [PATCH] nfsd4: handle failure to find backchannel J. Bruce Fields @ 2014-07-12 14:02 ` Kinglong Mee 2014-07-16 22:05 ` J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: Kinglong Mee @ 2014-07-12 14:02 UTC (permalink / raw) To: J. Bruce Fields, linux-nfs, kinglongmee On 7/12/2014 05:16, J. Bruce Fields wrote: > The local variable "ses" will be left NULL here in the case we fail to > find a connection. Spotted by a coverity scan. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 2c73cae9899d..fe22cd5c42d3 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb) > } > spin_unlock(&clp->cl_lock); > > + if (!c) > + goto out_no_connection; Setting err to -EINVAL maybe better. Otherwise, nfsd4_mark_cb_down will be called with err == 0. > err = setup_callback_client(clp, &conn, ses); setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL. thanks, Kinglong Mee > - if (err) { > - nfsd4_mark_cb_down(clp, err); > - return; > - } > + if (err) > + goto out_no_connection; > /* Yay, the callback channel's back! Restart any callbacks: */ > list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client) > run_nfsd4_cb(cb); > + return; > +out_no_connection: > + nfsd4_mark_cb_down(clp, err); > + return; > } > > static void nfsd4_do_callback_rpc(struct work_struct *w) > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd4: handle failure to find backchannel 2014-07-12 14:02 ` Kinglong Mee @ 2014-07-16 22:05 ` J. Bruce Fields 2014-07-17 13:24 ` Kinglong Mee 0 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2014-07-16 22:05 UTC (permalink / raw) To: Kinglong Mee; +Cc: linux-nfs On Sat, Jul 12, 2014 at 10:02:17PM +0800, Kinglong Mee wrote: > On 7/12/2014 05:16, J. Bruce Fields wrote: > > The local variable "ses" will be left NULL here in the case we fail to > > find a connection. Spotted by a coverity scan. > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index 2c73cae9899d..fe22cd5c42d3 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb) > > } > > spin_unlock(&clp->cl_lock); > > > > + if (!c) > > + goto out_no_connection; > > Setting err to -EINVAL maybe better. > Otherwise, nfsd4_mark_cb_down will be called with err == 0. > > > err = setup_callback_client(clp, &conn, ses); > > setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL. Thanks, yes, after looking over this carefully I don't believe we can call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so this is just a false positive from coverity. Thanks for the review! --b. > > thanks, > Kinglong Mee > > > - if (err) { > > - nfsd4_mark_cb_down(clp, err); > > - return; > > - } > > + if (err) > > + goto out_no_connection; > > /* Yay, the callback channel's back! Restart any callbacks: */ > > list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client) > > run_nfsd4_cb(cb); > > + return; > > +out_no_connection: > > + nfsd4_mark_cb_down(clp, err); > > + return; > > } > > > > static void nfsd4_do_callback_rpc(struct work_struct *w) > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd4: handle failure to find backchannel 2014-07-16 22:05 ` J. Bruce Fields @ 2014-07-17 13:24 ` Kinglong Mee 2014-07-17 15:13 ` J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: Kinglong Mee @ 2014-07-17 13:24 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On 7/17/2014 06:05, J. Bruce Fields wrote: > On Sat, Jul 12, 2014 at 10:02:17PM +0800, Kinglong Mee wrote: >> On 7/12/2014 05:16, J. Bruce Fields wrote: >>> The local variable "ses" will be left NULL here in the case we fail to >>> find a connection. Spotted by a coverity scan. >>> >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >>> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >>> index 2c73cae9899d..fe22cd5c42d3 100644 >>> --- a/fs/nfsd/nfs4callback.c >>> +++ b/fs/nfsd/nfs4callback.c >>> @@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb) >>> } >>> spin_unlock(&clp->cl_lock); >>> >>> + if (!c) >>> + goto out_no_connection; >> >> Setting err to -EINVAL maybe better. >> Otherwise, nfsd4_mark_cb_down will be called with err == 0. >> >>> err = setup_callback_client(clp, &conn, ses); >> >> setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL. > > Thanks, yes, after looking over this carefully I don't believe we can > call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so > this is just a false positive from coverity. ses and conn->cb_xprt will be set in the same condition before calling setup_callback_client, 996 c = __nfsd4_find_backchannel(clp); 997 if (c) { 998 svc_xprt_get(c->cn_xprt); 999 conn.cb_xprt = c->cn_xprt; 1000 ses = c->cn_session; 1001 } 1002 spin_unlock(&clp->cl_lock); 1003 1004 err = setup_callback_client(clp, &conn, ses); ses and conn->cb_xprt will be NULL or non-NULL in the same time, so that, call setup_calback_client with ses NULL but conn->cb_xprt non-NULL will not appear. thanks, Kinglong Mee >>> - if (err) { >>> - nfsd4_mark_cb_down(clp, err); >>> - return; >>> - } >>> + if (err) >>> + goto out_no_connection; >>> /* Yay, the callback channel's back! Restart any callbacks: */ >>> list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client) >>> run_nfsd4_cb(cb); >>> + return; >>> +out_no_connection: >>> + nfsd4_mark_cb_down(clp, err); >>> + return; >>> } >>> >>> static void nfsd4_do_callback_rpc(struct work_struct *w) >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd4: handle failure to find backchannel 2014-07-17 13:24 ` Kinglong Mee @ 2014-07-17 15:13 ` J. Bruce Fields 2014-07-18 0:01 ` Kinglong Mee 0 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2014-07-17 15:13 UTC (permalink / raw) To: Kinglong Mee; +Cc: linux-nfs On Thu, Jul 17, 2014 at 09:24:13PM +0800, Kinglong Mee wrote: > On 7/17/2014 06:05, J. Bruce Fields wrote: > > On Sat, Jul 12, 2014 at 10:02:17PM +0800, Kinglong Mee wrote: > >> On 7/12/2014 05:16, J. Bruce Fields wrote: > >>> The local variable "ses" will be left NULL here in the case we fail to > >>> find a connection. Spotted by a coverity scan. > >>> > >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> > >>> > >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > >>> index 2c73cae9899d..fe22cd5c42d3 100644 > >>> --- a/fs/nfsd/nfs4callback.c > >>> +++ b/fs/nfsd/nfs4callback.c > >>> @@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb) > >>> } > >>> spin_unlock(&clp->cl_lock); > >>> > >>> + if (!c) > >>> + goto out_no_connection; > >> > >> Setting err to -EINVAL maybe better. > >> Otherwise, nfsd4_mark_cb_down will be called with err == 0. > >> > >>> err = setup_callback_client(clp, &conn, ses); > >> > >> setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL. > > > > Thanks, yes, after looking over this carefully I don't believe we can > > call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so > > this is just a false positive from coverity. > > ses and conn->cb_xprt will be set in the same condition before > calling setup_callback_client, > > 996 c = __nfsd4_find_backchannel(clp); > 997 if (c) { > 998 svc_xprt_get(c->cn_xprt); > 999 conn.cb_xprt = c->cn_xprt; > 1000 ses = c->cn_session; > 1001 } > 1002 spin_unlock(&clp->cl_lock); > 1003 > 1004 err = setup_callback_client(clp, &conn, ses); > > ses and conn->cb_xprt will be NULL or non-NULL in the same time, > so that, call setup_calback_client with ses NULL but conn->cb_xprt non-NULL will not appear. Agreed but you also need to check what happens in the case where c is NULL. This is much more confusing than necessary. But I'm inclined to hold off on any cleanup here while jlayton's patches are still pending. --b. > > thanks, > Kinglong Mee > > >>> - if (err) { > >>> - nfsd4_mark_cb_down(clp, err); > >>> - return; > >>> - } > >>> + if (err) > >>> + goto out_no_connection; > >>> /* Yay, the callback channel's back! Restart any callbacks: */ > >>> list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client) > >>> run_nfsd4_cb(cb); > >>> + return; > >>> +out_no_connection: > >>> + nfsd4_mark_cb_down(clp, err); > >>> + return; > >>> } > >>> > >>> static void nfsd4_do_callback_rpc(struct work_struct *w) > >>> -- > >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > >>> the body of a message to majordomo@vger.kernel.org > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > >>> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd4: handle failure to find backchannel 2014-07-17 15:13 ` J. Bruce Fields @ 2014-07-18 0:01 ` Kinglong Mee 0 siblings, 0 replies; 6+ messages in thread From: Kinglong Mee @ 2014-07-18 0:01 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On 7/17/2014 23:13, J. Bruce Fields wrote: > On Thu, Jul 17, 2014 at 09:24:13PM +0800, Kinglong Mee wrote: >> On 7/17/2014 06:05, J. Bruce Fields wrote: >>> On Sat, Jul 12, 2014 at 10:02:17PM +0800, Kinglong Mee wrote: >>>> On 7/12/2014 05:16, J. Bruce Fields wrote: >>>>> The local variable "ses" will be left NULL here in the case we fail to >>>>> find a connection. Spotted by a coverity scan. >>>>> >>>>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >>>>> >>>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >>>>> index 2c73cae9899d..fe22cd5c42d3 100644 >>>>> --- a/fs/nfsd/nfs4callback.c >>>>> +++ b/fs/nfsd/nfs4callback.c >>>>> @@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb) >>>>> } >>>>> spin_unlock(&clp->cl_lock); >>>>> >>>>> + if (!c) >>>>> + goto out_no_connection; >>>> >>>> Setting err to -EINVAL maybe better. >>>> Otherwise, nfsd4_mark_cb_down will be called with err == 0. >>>> >>>>> err = setup_callback_client(clp, &conn, ses); >>>> >>>> setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL. >>> >>> Thanks, yes, after looking over this carefully I don't believe we can >>> call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so >>> this is just a false positive from coverity. >> >> ses and conn->cb_xprt will be set in the same condition before >> calling setup_callback_client, >> >> 996 c = __nfsd4_find_backchannel(clp); >> 997 if (c) { >> 998 svc_xprt_get(c->cn_xprt); >> 999 conn.cb_xprt = c->cn_xprt; >> 1000 ses = c->cn_session; >> 1001 } >> 1002 spin_unlock(&clp->cl_lock); >> 1003 >> 1004 err = setup_callback_client(clp, &conn, ses); >> >> ses and conn->cb_xprt will be NULL or non-NULL in the same time, >> so that, call setup_calback_client with ses NULL but conn->cb_xprt non-NULL will not appear. > > Agreed but you also need to check what happens in the case where c is > NULL. > > This is much more confusing than necessary. But I'm inclined to hold > off on any cleanup here while jlayton's patches are still pending. Got it. I also have some cleanup, will send after jlayton's patches merged. thanks, Kinglong Mee > > --b. > >> >> thanks, >> Kinglong Mee >> >>>>> - if (err) { >>>>> - nfsd4_mark_cb_down(clp, err); >>>>> - return; >>>>> - } >>>>> + if (err) >>>>> + goto out_no_connection; >>>>> /* Yay, the callback channel's back! Restart any callbacks: */ >>>>> list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client) >>>>> run_nfsd4_cb(cb); >>>>> + return; >>>>> +out_no_connection: >>>>> + nfsd4_mark_cb_down(clp, err); >>>>> + return; >>>>> } >>>>> >>>>> static void nfsd4_do_callback_rpc(struct work_struct *w) >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-18 0:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-11 21:16 [PATCH] nfsd4: handle failure to find backchannel J. Bruce Fields 2014-07-12 14:02 ` Kinglong Mee 2014-07-16 22:05 ` J. Bruce Fields 2014-07-17 13:24 ` Kinglong Mee 2014-07-17 15:13 ` J. Bruce Fields 2014-07-18 0:01 ` Kinglong Mee
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox