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 <[email protected]>
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)
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 <[email protected]>
>
> 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 [email protected]
> 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 <[email protected]>
> >
> > 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 [email protected]
> > 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 <[email protected]>
>>>>>
>>>>> 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 [email protected]
>>>>> 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 <[email protected]>
>>>
>>> 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 [email protected]
>>> 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 <[email protected]>
> >>>
> >>> 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 [email protected]
> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>>
> >