| Message ID | 20140711211609.GA13656@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- 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
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 > > -- 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
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 >>> > -- 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
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 > >>> > > -- 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
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 >>>>> >>> > -- 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
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)
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> -- 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