From patchwork Fri Mar 10 01:37:43 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kinglong Mee X-Patchwork-Id: 9614463 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 37699604D9 for ; Fri, 10 Mar 2017 01:38:02 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1D38E286F1 for ; Fri, 10 Mar 2017 01:38:02 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0EE34286EF; Fri, 10 Mar 2017 01:38:02 +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_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, 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 D6FEA286EF for ; Fri, 10 Mar 2017 01:38:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751383AbdCJBh7 (ORCPT ); Thu, 9 Mar 2017 20:37:59 -0500 Received: from mail-it0-f66.google.com ([209.85.214.66]:33603 "EHLO mail-it0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750816AbdCJBh6 (ORCPT ); Thu, 9 Mar 2017 20:37:58 -0500 Received: by mail-it0-f66.google.com with SMTP id g138so94801itb.0 for ; Thu, 09 Mar 2017 17:37:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=GAU4YurwnUX6RboARE4CkZ3liX8ktM8eqsM4st8SBaU=; b=mNnG7aSIPFyBn+JWmkTOJrDD0HPSL6aVDGsFt+gkcG2X5uh2rJwgK8I50trgg5Fto3 txzQuC3U0FTPkchYla1XIvaRrsXZdqP/hWZoluoTzdzBSMoCsfNgeHcDMuZXWzKp9xyL bGwyCA/2y9MXbhNAGSkKpLCuLPm0vAhX3m8/G/wCCspTjJPS9GLZhaDUpN0BaP6IX2OG 4baljIk2hoWPmgzlgC9dfpTJsimz86iDbDStDBA7CDeZx4HXtWxC8vmi74B1a86ymMhQ ymUfdCmP6j74h96QSX+tH0x8AMqhV6Amda0bjcCXP7KBa0mY2fAi/cZSfm5Y7KMkNIq+ u65g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=GAU4YurwnUX6RboARE4CkZ3liX8ktM8eqsM4st8SBaU=; b=PkCpVQg17XZLS76Edfj5rYmni0eV9aaM1BBC/NzN9iL/L3gy4CpqG5C9bI1U/hiV1u 1ZRW+NyRQdrkOZcazPwf7f/3NyxmVsINmp1h/phMKG2lGDlICn0+3hq7Sh2ZvRCchfXS ilLrmyWPZAfqc5fqEnRzELajWpoP152cyvJw46UadoL16JJMgyrAicJvcsCYBv91z5jS 8kMBsgea2l8lbSXMA2GwsqQC1A+RBZXxL84RlgBAZFyoPhBkXoYTfOywRhYqpdcqvFJX IcdruMxo/oXJiwNIHFSeHH7HpN3vcPkqMTveI6/2WfLggjnTYwdKijdDRdmRYqIv9RhW ymzg== X-Gm-Message-State: AFeK/H2eJfJCPBehDwPbjrTnzZu6+TOq1kEISk3ZHSHZcDdO2SwwswftvyiyCvw+XdNN0A== X-Received: by 10.36.173.90 with SMTP id a26mr151859itj.27.1489109877374; Thu, 09 Mar 2017 17:37:57 -0800 (PST) Received: from [192.168.0.106] ([183.228.28.193]) by smtp.gmail.com with ESMTPSA id y7sm342941itc.27.2017.03.09.17.37.55 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Mar 2017 17:37:56 -0800 (PST) Subject: Re: [PATCH] SUNRPC: Make sure authorize svc when meeting SVC_CLOSE To: "J. Bruce Fields" References: <090f6673-1bb3-d626-e27e-6be5afcda782@gmail.com> <20170118222028.GB4272@fieldses.org> Cc: linux-nfs@vger.kernel.org, Chuck Lever , Kinglong Mee From: Kinglong Mee Message-ID: <2a4525db-8ef0-bd33-5119-7d0046345827@gmail.com> Date: Fri, 10 Mar 2017 09:37:43 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <20170118222028.GB4272@fieldses.org> 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 1/19/2017 06:20, J. Bruce Fields wrote: > On Mon, Jan 16, 2017 at 08:23:37AM +0800, Kinglong Mee wrote: >> Commit 4d712ef1db05 "svcauth_gss: Close connection when >> dropping an incoming message" will close connection, >> but forget authorizing the svc when meeting SVC_CLOSE. >> >> That, there will be an module reference to sunrpc, >> and some memory leak. >> >> When mounting an nfs filesystem, the reference leak increase one. > > Thanks, applying for 4.10. > > (I'm not too happy with this function--e.g. it'd be easier to avoid this > sort of bug if we had a single unavoidable common exit that always > called svc_authenticate. > > But I'm not sure of the best cleanup on a quick look. And this is a > simple bugfix. Maybe we could add some cleanup on top for 4.11.) Hi Bruce, I don't see this patch in the latest linux kernel (4.11.0-rc1+), what's your plan about it? Ps, for the cleanup, I have a draft for it, but it's a good one. thanks, Kinglong Mee ----------------------------------------------------------------------------- --- 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/net/sunrpc/svc.c b/net/sunrpc/svc.c index 44e7d49..a390842 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -1086,6 +1086,30 @@ 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 +#define FMT_ERR_ENCODE(rpc_stat) do { \ + svc_printk(rqstp, "svc: decode error ##rpc_stat (%s:%d)\n", \ + __FILE__, __LINE__); \ + serv->sv_stats->rpcbadfmt++; \ + svc_putnl(resv, (rpc_stat)); \ +} while (0) + +#define VERS_ERR_ENCODE(lovers, hivers) do { \ + FMT_ERR_ENCODE(RPC_PROG_MISMATCH); \ + svc_putnl(resv, (lovers)); \ + svc_putnl(resv, (hivers)); \ +} while (0) + +#define AUTH_ERR_ENCODE(auth_stat) do { \ + svc_printk(rqstp, "svc: authentication error %d (%s:%d)\n", \ + ntohl(auth_stat), __FILE__, __LINE__); \ + serv->sv_stats->rpcbadauth++; \ + /* Restore write pointer to location of accept status: */ \ + xdr_ressize_check(rqstp, reply_statp); \ + svc_putnl(resv, 1); /* REJECT */ \ + svc_putnl(resv, 1); /* AUTH_ERROR */ \ + svc_putnl(resv, ntohl(auth_stat)); /* status */ \ +} while (0) + /* * Common routine for processing the RPC request. */ @@ -1099,14 +1123,15 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) kxdrproc_t xdr; __be32 *statp; u32 prog, vers, proc; - __be32 auth_stat, rpc_stat; + __be32 auth_stat; int auth_res; __be32 *reply_statp; - rpc_stat = rpc_success; - - if (argv->iov_len < 6*4) - goto err_short_len; + if (argv->iov_len < 6*4) { + svc_printk(rqstp, "short len %zd, dropping request\n", + argv->iov_len); + goto close; + } /* Will be turned off only in gss privacy case: */ set_bit(RQ_SPLICE_OK, &rqstp->rq_flags); @@ -1124,8 +1149,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) /* First words of reply: */ svc_putnl(resv, 1); /* REPLY */ - if (vers != 2) /* RPC version number */ - goto err_bad_rpc; + if (vers != 2) { /* RPC version number */ + serv->sv_stats->rpcbadfmt++; + svc_putnl(resv, 1); /* REJECT */ + svc_putnl(resv, 0); /* RPC_MISMATCH */ + svc_putnl(resv, 2); /* Only RPCv2 supported */ + svc_putnl(resv, 2); + goto sendit; + } /* Save position in case we later decide to reject: */ reply_statp = resv->iov_base + resv->iov_len; @@ -1155,12 +1186,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) case SVC_OK: break; case SVC_GARBAGE: - goto err_garbage; + FMT_ERR_ENCODE(RPC_GARBAGE_ARGS); + goto sendit; case SVC_SYSERR: - rpc_stat = rpc_system_err; - goto err_bad; + FMT_ERR_ENCODE(RPC_SYSTEM_ERR); + goto sendit; case SVC_DENIED: - goto err_bad_auth; + AUTH_ERR_ENCODE(auth_stat); + goto sendit; case SVC_CLOSE: /* * Makesure authorise svc if progp->pg_authenticate fail, @@ -1174,12 +1207,19 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) goto sendit; } - if (progp == NULL) - goto err_bad_prog; + if (progp == NULL) { + FMT_ERR_ENCODE(RPC_PROG_UNAVAIL); + svc_printk(rqstp, "svc: unknown program %d\n", prog); + goto sendit; + } if (vers >= progp->pg_nvers || - !(versp = progp->pg_vers[vers])) - goto err_bad_vers; + !(versp = progp->pg_vers[vers])) { + VERS_ERR_ENCODE(progp->pg_lovers, progp->pg_hivers); + svc_printk(rqstp, "unknown version (%d for prog %d, %s)\n", + vers, prog, progp->pg_name); + goto sendit; + } /* * Some protocol versions (namely NFSv4) require some form of @@ -1193,12 +1233,20 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) * fit. */ if (versp->vs_need_cong_ctrl && - !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags)) - goto err_bad_vers; + !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags)) { + VERS_ERR_ENCODE(progp->pg_lovers, progp->pg_hivers); + svc_printk(rqstp, "congestion control (%d for prog %d, %s)\n", + vers, prog, progp->pg_name); + goto sendit; + } procp = versp->vs_proc + proc; - if (proc >= versp->vs_nproc || !procp->pc_func) - goto err_bad_proc; + if (proc >= versp->vs_nproc || !procp->pc_func) { + FMT_ERR_ENCODE(RPC_PROC_UNAVAIL); + svc_printk(rqstp, "unknown procedure (%d)\n", proc); + goto sendit; + } + rqstp->rq_procinfo = procp; /* Syntactic check complete */ @@ -1225,8 +1273,10 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) if (!versp->vs_dispatch) { /* Decode arguments */ xdr = procp->pc_decode; - if (xdr && !xdr(rqstp, argv->iov_base, rqstp->rq_argp)) - goto err_garbage; + if (xdr && !xdr(rqstp, argv->iov_base, rqstp->rq_argp)) { + FMT_ERR_ENCODE(RPC_GARBAGE_ARGS); + goto sendit; + } *statp = procp->pc_func(rqstp, rqstp->rq_argp, rqstp->rq_resp); @@ -1240,12 +1290,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) if (*statp == rpc_autherr_badcred) { if (procp->pc_release) procp->pc_release(rqstp, NULL, rqstp->rq_resp); - goto err_bad_auth; + + AUTH_ERR_ENCODE(auth_stat); + goto sendit; } if (*statp == rpc_success && (xdr = procp->pc_encode) && !xdr(rqstp, resv->iov_base+resv->iov_len, rqstp->rq_resp)) { - dprintk("svc: failed to encode reply\n"); + svc_printk(rqstp, "svc: failed to encode reply\n"); /* serv->sv_stats->rpcsystemerr++; */ *statp = rpc_system_err; } @@ -1269,77 +1321,18 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv) if (procp->pc_encode == NULL) goto dropit; - - sendit: - if (svc_authorise(rqstp)) - goto close; - return 1; /* Caller can now send it */ - - dropit: - svc_authorise(rqstp); /* doesn't hurt to call this twice */ - dprintk("svc: svc_process dropit\n"); - return 0; - - close: +sendit: + if (!svc_authorise(rqstp)) + return 1; /* Caller can now send it */ +close: if (test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags)) svc_close_xprt(rqstp->rq_xprt); dprintk("svc: svc_process close\n"); return 0; - -err_short_len: - svc_printk(rqstp, "short len %zd, dropping request\n", - argv->iov_len); - goto close; - -err_bad_rpc: - serv->sv_stats->rpcbadfmt++; - svc_putnl(resv, 1); /* REJECT */ - svc_putnl(resv, 0); /* RPC_MISMATCH */ - svc_putnl(resv, 2); /* Only RPCv2 supported */ - svc_putnl(resv, 2); - goto sendit; - -err_bad_auth: - dprintk("svc: authentication failed (%d)\n", ntohl(auth_stat)); - serv->sv_stats->rpcbadauth++; - /* Restore write pointer to location of accept status: */ - xdr_ressize_check(rqstp, reply_statp); - svc_putnl(resv, 1); /* REJECT */ - svc_putnl(resv, 1); /* AUTH_ERROR */ - svc_putnl(resv, ntohl(auth_stat)); /* status */ - goto sendit; - -err_bad_prog: - dprintk("svc: unknown program %d\n", prog); - serv->sv_stats->rpcbadfmt++; - svc_putnl(resv, RPC_PROG_UNAVAIL); - goto sendit; - -err_bad_vers: - svc_printk(rqstp, "unknown version (%d for prog %d, %s)\n", - 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); - goto sendit; - -err_bad_proc: - svc_printk(rqstp, "unknown procedure (%d)\n", proc); - - serv->sv_stats->rpcbadfmt++; - svc_putnl(resv, RPC_PROC_UNAVAIL); - goto sendit; - -err_garbage: - svc_printk(rqstp, "failed to decode args\n"); - - rpc_stat = rpc_garbage_args; -err_bad: - serv->sv_stats->rpcbadfmt++; - svc_putnl(resv, ntohl(rpc_stat)); - goto sendit; +dropit: + svc_authorise(rqstp); /* doesn't hurt to call this twice */ + dprintk("svc: svc_process dropit\n"); + return 0; } /*