Re: [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf

From: Trond Myklebust <trondmy-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
To: Fields Bruce James <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
Cc: List Linux NFS Mailing
	<linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>,
	Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>,
	"linux-crypto-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-crypto-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf
Date: Tue, 25 Oct 2016 21:47:16 +0000	[thread overview]
Message-ID: <DB39073B-8C4B-48BC-8D46-7B6173966037@primarydata.com> (raw)
In-Reply-To: <20161025195930.GB5129-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>

> On Oct 25, 2016, at 15:59, Fields Bruce James <bfields@fieldses.org> wrote:
> 
> On Tue, Oct 25, 2016 at 07:34:43PM +0000, Trond Myklebust wrote:
>> 
>>> On Oct 25, 2016, at 14:45, J. Bruce Fields <bfields@fieldses.org> wrote:
>>> 
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>> 
>>> As of ac4e97abce9b "scatterlist: sg_set_buf() argument must be in linear
>>> mapping", sg_set_buf hits a BUG when make_checksum_v2->xdr_process_buf,
>>> among other callers, passes it memory on the stack.
>>> 
>>> We only need a scatterlist to pass this to the crypto code, and it seems
>>> like overkill to require kmalloc'd memory just to encrypt a few bytes,
>>> but for now this seems the best fix.
>>> 
>>> Note many of these callers are in the NFS write paths, so we shouldn't
>>> really be allocating GFP_KERNEL.  But we already have other allocations
>>> in these code paths.  A larger redesign may be necessary to allow
>>> allocations to be done earlier.
>> 
>> NACK…  I agree that there may already be borkage in RPCSEC_GSS-land, but that’s not a reason to be adding to the pile of things that need to be fixed… The allocations that lie in the RPC client’s encode/decode path do need to be GFP_NOFS….
> 
> OK.  Any disadvantage to keeping this patch with just GFP_NOFS
> allocations everywhere that might be in the write path?

It’s what we have to do everywhere else in the RPC client, so I can’t see why it should be a problem.