From patchwork Mon Mar 27 12:39:13 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 9646595 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 53350602D6 for ; Mon, 27 Mar 2017 12:39:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4830B28249 for ; Mon, 27 Mar 2017 12:39:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3CD692835B; Mon, 27 Mar 2017 12:39:41 +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.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID 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 78ECE28249 for ; Mon, 27 Mar 2017 12:39:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751973AbdC0Mjk (ORCPT ); Mon, 27 Mar 2017 08:39:40 -0400 Received: from mail-qt0-f194.google.com ([209.85.216.194]:34330 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752021AbdC0Mjj (ORCPT ); Mon, 27 Mar 2017 08:39:39 -0400 Received: by mail-qt0-f194.google.com with SMTP id x35so7316925qtc.1 for ; Mon, 27 Mar 2017 05:39:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=poochiereds-net.20150623.gappssmtp.com; s=20150623; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=m797BTM5D3dwaxlOwREmVzdULoGEkA90HCM6dAB87Yg=; b=o7vl1D7zqlS3dAPOFtbtpDS6rqSYpgTi2AC8b5UUGfCiDBnwgxQOTngFwXv9UC8kUP luvq2cCRRDS87bUB8bsvaNdY+rIguY35iDYNF+3AI+8UW6/JO3+y6Ji1YcRVL6R8+J05 W5zW/X/bKY/x5EVc/fVSBcgrfQUwF/BlAXrAYfU9jRNeEit/yq7Mbyf5vNtCUpyFwP2f oIy05vpOQylNGpJKcjQkZE35bUsXfd6GHBs3xqAK5NJenAyYHl6kVDe33uhMhPRZPCPw 1foVHn+UatDkTj7x10N35Z6ypRzwr1cCnS/6H7FviNSriJJfCX6estjUF5/WOxsPJFA2 i6+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=m797BTM5D3dwaxlOwREmVzdULoGEkA90HCM6dAB87Yg=; b=ZSCB572JLi2s3psCEujdLMVLv0l7TEH/Pbng+DNnEqUNrZK2CEQ73CbLzqmI9Wgd5x SHrwYMICqcYghzU/0VVirl9qSi4fVrnOpVofUtWfFxPZKXPFQO6ryXszel7o9DsWFtjs 2c/3NToGp+HVTHcLFONw/yHylS787RGklI9xXtYoTvt6swPGmrnd9whMsYP/0sWnjigk Lg2ExCMqnRvXZHWVxFw9LKawsEN5rp3/3qqBwAIUJA6MdQhdst5FaimGpWfz6Rqi5CLm RycHZFdpB+HZLp1adzAjQL1ZqzXqgK0XjA5l9Pc2yYtSbsWtv+67G6UoOVYBakxsVtzz ipug== X-Gm-Message-State: AFeK/H13js3H1KGw4Es6tfl1m7fPB71RogbV314Mmf4TdT/kWzBOUzFbOW0Mgsnpiqbr7A== X-Received: by 10.200.55.181 with SMTP id d50mr22801710qtc.140.1490618357622; Mon, 27 Mar 2017 05:39:17 -0700 (PDT) Received: from cpe-2606-A000-1125-405B-1A5E-FFF-FE12-8671.dyn6.twc.com (cpe-2606-A000-1125-405B-1A5E-FFF-FE12-8671.dyn6.twc.com. [2606:a000:1125:405b:1a5e:fff:fe12:8671]) by smtp.gmail.com with ESMTPSA id s46sm246356qtc.62.2017.03.27.05.39.16 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 27 Mar 2017 05:39:16 -0700 (PDT) Message-ID: <1490618353.6879.3.camel@poochiereds.net> Subject: Re: [PATCH] svcrdma: set XPT_CONG_CTRL flag for bc xprt From: Jeff Layton To: Chuck Lever , Chuck Lever Cc: bfields@fieldses.org, linux-nfs@vger.kernel.org Date: Mon, 27 Mar 2017 08:39:13 -0400 In-Reply-To: <1490612833.2808.1.camel@poochiereds.net> References: <20170326231254.1319.26075.stgit@manet.1015granger.net> <1490577699.6879.1.camel@poochiereds.net> <62622BEB-E234-4035-94FE-0C34E00693AE@gmail.com> <1490612833.2808.1.camel@poochiereds.net> X-Mailer: Evolution 3.22.6 (3.22.6-1.fc25) 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, 2017-03-27 at 07:07 -0400, Jeff Layton wrote: > On Sun, 2017-03-26 at 22:41 -0400, Chuck Lever wrote: > > > On Mar 26, 2017, at 10:38 PM, Chuck Lever wrote: > > > > > > Hey Jeff- > > > > > > > > > > > On Mar 26, 2017, at 9:21 PM, Jeff Layton wrote: > > > > > > > > > > On Sun, 2017-03-26 at 19:27 -0400, Chuck Lever wrote: > > > > > Same change as Kinglong Mee's fix for the TCP backchannel service. > > > > > > > > > > > > > Good catch. I guess I didn't do a good job of hunting down all of the > > > > transports where this needed to be set. I'll give them another pass > > > > again tomorrow to make sure I didn't miss any others. > > > > > > > > > Fixes: 5283b03ee5cd ("nfs/nfsd/sunrpc: enforce transport...") > > > > > Signed-off-by: Chuck Lever > > > > > --- > > > > > Some (perhaps late) review comments on 5283b03ee5cd: > > > > > > > > > > I have reservations about returning RPC_PROG_MISMATCH in this case. > > > > > RPC_PROG_UNAVAIL is more sensible. But the use of UDP with NFSv4 is > > > > > not an RPC-level error, thus reporting the problem here seems like a > > > > > layering violation. > > > > > > > > > > I'm not sure why an explicit check is needed: if the server isn't > > > > > listening on UDP, wouldn't clients see a transport-level rejection > > > > > (like ECONNREFUSED)? > > > > > > > > > > > > > Sure, if the server isn't listening on UDP... > > > > > > > > The point of that patch is to enforce not allowing v4 over UDP when the > > > > server is listening on UDP to serve earlier versions. > > > > > > > > As far as the error...From RFC 5531: > > > > > > > > PROG_UNAVAIL = 1, /* remote hasn't exported program */ > > > > PROG_MISMATCH = 2, /* remote can't support version # */ > > > > > > > > Consider the case where the server is listening on both TCP and UDP, > > > > and is serving both v3 and v4. Someone tries to send a v4 RPC over UDP. > > > > > > > > The RPC program in that case (nfs) is supported over UDP, but the > > > > version (v4) is not. So I disagree here. PROG_MISMATCH seems like the > > > > better fit to me. > > > > > > Then the server should report the correct version range in the > > > rejection. The RPC response I saw on the wire claimed that 4 > > > was the maximum supported version. > > Of course, versions 2 and 3 do not make sense for > > the backchannel. So I'm not sure what you would report > > in that case. > > > > Yeah, that's clearly a bug. The problem is that we currently track > min/max versions on a per-program basis, but really we need to track > them per-program + per-transport. > > Another way to fix it would be to set that info more dynamically at the > time of the error. Walk the pg_vers array and if we're on a non- > congestion control transport, skip any entries that require it. > Something like this patch maybe? Builds but is otherwise untested. It might not DTRT though in the (nonsensical) case where you have a server that is listening on UDP but doesn't support v2 or v3. Not sure I really care about that too much. [PATCH] sunrpc: dynamically set versions in PROG_MISMATCH error reply Signed-off-by: Jeff Layton --- include/linux/sunrpc/svc.h | 2 -- net/sunrpc/svc.c | 47 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h index e770abeed32d..f19321cfcf21 100644 --- a/include/linux/sunrpc/svc.h +++ b/include/linux/sunrpc/svc.h @@ -381,8 +381,6 @@ struct svc_deferred_req { struct svc_program { struct svc_program * pg_next; /* other programs (same xprt) */ u32 pg_prog; /* program number */ - unsigned int pg_lovers; /* lowest version */ - unsigned int pg_hivers; /* highest version */ unsigned int pg_nvers; /* number of versions */ struct svc_version ** pg_vers; /* version array */ char * pg_name; /* service name */ diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index a08aeb56b8e4..c81f68064313 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -441,18 +441,13 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools, serv->sv_ops = ops; xdrsize = 0; while (prog) { - prog->pg_lovers = prog->pg_nvers-1; for (vers=0; verspg_nvers ; vers++) - if (prog->pg_vers[vers]) { - prog->pg_hivers = vers; - if (prog->pg_lovers > vers) - prog->pg_lovers = vers; - if (prog->pg_vers[vers]->vs_xdrsize > xdrsize) - xdrsize = prog->pg_vers[vers]->vs_xdrsize; - } + if (prog->pg_vers[vers] && + prog->pg_vers[vers]->vs_xdrsize > xdrsize) + xdrsize = prog->pg_vers[vers]->vs_xdrsize; prog = prog->pg_next; } - serv->sv_xdrsize = xdrsize; + serv->sv_xdrsize = xdrsize; INIT_LIST_HEAD(&serv->sv_tempsocks); INIT_LIST_HEAD(&serv->sv_permsocks); init_timer(&serv->sv_temptimer); @@ -1086,6 +1081,36 @@ void svc_printk(struct svc_rqst *rqstp, const char *fmt, ...) static __printf(2,3) void svc_printk(struct svc_rqst *rqstp, const char *fmt, ...) {} #endif +static void +svc_set_prog_mismatch(struct svc_rqst *rqstp, struct svc_program *prog, + struct kvec *resv) +{ + unsigned int vers, hi = 0, lo = prog->pg_nvers - 1; + struct svc_version *versp; + + for (vers = 0; vers < prog->pg_nvers ; vers++) { + versp = prog->pg_vers[vers]; + if (!versp) + continue; + + /* + * Don't report this version if it needs congestion control + * and the xprt doesn't provide it. + */ + if (versp->vs_need_cong_ctrl && + !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags)) + continue; + + hi = vers; + if (lo > vers) + lo = vers; + } + + svc_putnl(resv, RPC_PROG_MISMATCH); + svc_putnl(resv, lo); + svc_putnl(resv, hi); +} + /* * Common routine for processing the RPC request. */ @@ -1315,9 +1340,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) vers, prog, progp->pg_name); serv->sv_stats->rpcbadfmt++; - svc_putnl(resv, RPC_PROG_MISMATCH); - svc_putnl(resv, progp->pg_lovers); - svc_putnl(resv, progp->pg_hivers); + svc_set_prog_mismatch(rqstp, progp, resv); goto sendit; err_bad_proc: