From patchwork Tue May 23 00:52:38 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: NeilBrown X-Patchwork-Id: 9741619 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B80C560388 for ; Tue, 23 May 2017 00:52:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AA84326E51 for ; Tue, 23 May 2017 00:52:48 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9C61128676; Tue, 23 May 2017 00:52:48 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2075826E51 for ; Tue, 23 May 2017 00:52:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758076AbdEWAwr (ORCPT ); Mon, 22 May 2017 20:52:47 -0400 Received: from mx2.suse.de ([195.135.220.15]:43792 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752580AbdEWAwq (ORCPT ); Mon, 22 May 2017 20:52:46 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 4D360ACEA; Tue, 23 May 2017 00:52:45 +0000 (UTC) From: NeilBrown To: Steve Dickson , Linux NFS Mailing list Date: Tue, 23 May 2017 10:52:38 +1000 Subject: Re: [PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used. In-Reply-To: <5a0d2640-ec93-279b-9113-228994283923@RedHat.com> References: <20170519222510.31205-1-steved@redhat.com> <87inktk2yg.fsf@notabene.neil.brown.name> <5a0d2640-ec93-279b-9113-228994283923@RedHat.com> Message-ID: <877f18jsx5.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, May 22 2017, Steve Dickson wrote: > On 05/21/2017 11:03 PM, NeilBrown wrote: >> On Fri, May 19 2017, Steve Dickson wrote: >> >>> When the pseudo root is set with fsid=0, explicit >>> v4 mounts (via the -o flag) should fail when >>> the incorrect export is tried instead of rolling >>> back to v3. >> >> Hi Steve, >> I think this patch makes sense, but the above description doesn't. >> Where does fsid=0 fit in anywhere here? > It sets the export to be the pseudo root > /home *(rw,fsid=0,sec=sys:krb5:krb5i:krb5p) > > so when then that export using either -t nfs4 or -o v4 > mount -o v4.0 127.0.0.1:/home /mnt > > the mount should fail instead of rolling back to v3 > Basically its be used to cause the error. > >> >> I think you want to say >> >> When the protocol is set with "-t nfs4", we should behave just like >> with do with "-o vers=4" and not fall back to v3. > Actually the first patch fixes the -o vers=4 case since > that too was rolling back to v3 in the above scenario > >> >> Is that what you were really trying to say? > How about > > When the protocol is set the "-o v4" flag, > and the mount fails due to a pseudo root > issue, the mount should fail not, roll > back to v3. Better, but I don't think the pseudo root has any relevance. If you ask for v4, you should get v4, not v3. How the server may or may not behave differently between v3 and v4 is irrelevant. You should get what you asked for. But now that I look at the code again... I don't understand. You say this is for the "-o v4" case. In that case, the current code in nfs_nfs_version() will find the "v4" entry in nfs_version_opttbl[] and set version_val = "4"; version->v_mode = V_SPECIFIC; then version_major = 4; then as *cptr == '\0' and ->major > 4, version->v_mode = V_GENERAL; Your change skips that last step so it finished with v_mod == V_SPECIFIC. According to the extra comments you have added for the modes: >>> + V_GENERAL, /* single digit => 4 */ >>> + V_SPECIFIC, /* single digit < 4 or decimal included */ And it seems to me that "v4" should be V_GENERAL, not V_SPECIFIC. So I think the current code is correct. Except... nfs_try_mount() will then call nfs_autonegotiate(), and nfs_autonegotiate() isn't very consistent about how it interprets V_GENERAL and V_SPECIFIC. For EINVAL, it gets the difference right, for other errors it doesn't. So I think that this is the fix you want: I haven't even compile-tested of course :-) Thanks, NeilBrown > > steved. > >> >> Thanks, >> NeilBrown >> >> >>> >>> Signed-off-by: Steve Dickson >>> --- >>> utils/mount/network.c | 3 ++- >>> utils/mount/network.h | 8 ++++---- >>> 2 files changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/utils/mount/network.c b/utils/mount/network.c >>> index 281e935..e39263e 100644 >>> --- a/utils/mount/network.c >>> +++ b/utils/mount/network.c >>> @@ -1299,7 +1299,8 @@ nfs_nfs_version(struct mount_options *options, struct nfs_version *version) >>> if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val) >>> goto ret_error; >>> version->v_mode = V_SPECIFIC; >>> - } else if (version->major > 3 && *cptr == '\0') >>> + } else if (version->major > 3 && *cptr == '\0' && >>> + version->v_mode == V_DEFAULT) /* v_mode has not been set */ >>> version->v_mode = V_GENERAL; >>> >>> if (*cptr != '\0') >>> diff --git a/utils/mount/network.h b/utils/mount/network.h >>> index 9cc5dec..45e2b24 100644 >>> --- a/utils/mount/network.h >>> +++ b/utils/mount/network.h >>> @@ -58,10 +58,10 @@ int clnt_ping(struct sockaddr_in *, const unsigned long, >>> struct mount_options; >>> >>> enum { >>> - V_DEFAULT = 0, >>> - V_GENERAL, >>> - V_SPECIFIC, >>> - V_PARSE_ERR, >>> + V_DEFAULT = 0, /* not set */ >>> + V_GENERAL, /* single digit => 4 */ >>> + V_SPECIFIC, /* single digit < 4 or decimal included */ >>> + V_PARSE_ERR, /* miss all others */ >>> }; >>> >>> struct nfs_version { >>> -- >>> 2.9.4 >>> >>> -- >>> 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/utils/mount/stropts.c b/utils/mount/stropts.c index 0fbb37569ef9..98cf813fe439 100644 --- a/utils/mount/stropts.c +++ b/utils/mount/stropts.c @@ -838,9 +838,6 @@ check_result: case EINVAL: /* A less clear indication that our client * does not support NFSv4 minor version. */ - if (mi->version.v_mode == V_GENERAL && - mi->version.minor == 0) - return result; if (mi->version.v_mode != V_SPECIFIC) { if (mi->version.minor > 0) { mi->version.minor--; @@ -862,6 +859,9 @@ check_result: /* UDP-Only servers won't support v4, but maybe it * just isn't ready yet. So try v3, but double-check * with rpcbind for v4. */ + if (mi->version.v_mode == V_GENERAL) + /* Mustn't try v2,v3 */ + return result; result = nfs_try_mount_v3v2(mi, TRUE); if (result == 0 && errno == EAGAIN) { /* v4 server seems to be registered now. */ @@ -878,6 +878,9 @@ check_result: } fall_back: + if (mi->version.v_mode == V_GENERAL) + /* v2,3 fallback not allowed */ + return result; return nfs_try_mount_v3v2(mi, FALSE); }