Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-pa0-f45.google.com ([209.85.220.45]:51127 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752554AbaGRACH (ORCPT ); Thu, 17 Jul 2014 20:02:07 -0400 Received: by mail-pa0-f45.google.com with SMTP id eu11so4269022pac.18 for ; Thu, 17 Jul 2014 17:02:06 -0700 (PDT) Message-ID: <53C863F4.8090408@gmail.com> Date: Fri, 18 Jul 2014 08:01:56 +0800 From: Kinglong Mee MIME-Version: 1.0 To: "J. Bruce Fields" CC: linux-nfs@vger.kernel.org Subject: Re: [PATCH] nfsd4: handle failure to find backchannel References: <20140711211609.GA13656@fieldses.org> <53C13FE9.3080503@gmail.com> <20140716220526.GG2397@fieldses.org> <53C7CE7D.9070904@gmail.com> <20140717151337.GA20073@fieldses.org> In-Reply-To: <20140717151337.GA20073@fieldses.org> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 >>>>> >>>>> 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 >>>>> >>> >