Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-pd0-f171.google.com ([209.85.192.171]:53023 "EHLO mail-pd0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751738AbbALTnG (ORCPT ); Mon, 12 Jan 2015 14:43:06 -0500 Received: by mail-pd0-f171.google.com with SMTP id y13so32127040pdi.2 for ; Mon, 12 Jan 2015 11:43:05 -0800 (PST) Date: Mon, 12 Jan 2015 11:43:02 -0800 From: Omar Sandoval To: Trond Myklebust Cc: Linux NFS Mailing List , Linux Kernel mailing list , Al Viro , Devel FS Linux Subject: Re: [PATCH RESEND] nfs: prevent truncate on active swapfile Message-ID: <20150112194302.GA27355@mew.dhcp4.washington.edu> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: On Sat, Jan 10, 2015 at 05:08:03PM -0500, Trond Myklebust wrote: > Hi Omar, > > On Thu, Jan 8, 2015 at 4:18 AM, Omar Sandoval wrote: > > Most filesystems prevent truncation of an active swapfile by way of > > inode_newsize_ok, called from inode_change_ok. NFS doesn't call either > > from nfs_setattr, presumably because most of these checks are expected > > to be done server-side. However, the IS_SWAPFILE check can only be done > > client-side, and truncating a swapfile can't possibly be good. > > > > Signed-off-by: Omar Sandoval > > --- > > Hi, Trond, > > > > Now that the holidays are over, could you take a look at this? It was > > generated against v3.19-rc3. > > > > Thanks! > > > > fs/nfs/inode.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > > index 4bffe63..9205513 100644 > > --- a/fs/nfs/inode.c > > +++ b/fs/nfs/inode.c > > @@ -506,10 +506,15 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr) > > attr->ia_valid &= ~ATTR_MODE; > > > > if (attr->ia_valid & ATTR_SIZE) { > > + loff_t i_size; > > + > > BUG_ON(!S_ISREG(inode->i_mode)); > > > > - if (attr->ia_size == i_size_read(inode)) > > + i_size = i_size_read(inode); > > + if (attr->ia_size == i_size) > > attr->ia_valid &= ~ATTR_SIZE; > > + else if (attr->ia_size < i_size && IS_SWAPFILE(inode)) > > + return -ETXTBSY; > > } > > > > /* Optimization: if the end result is no change, don't RPC */ > > -- > > 2.2.1 > > > > I agree that truncating a swap file is bad, however as you point out, > this really only addresses the case on the client that knows about > this being a swap file. > I'll take the patch, Thanks, I appreciate it. > but I'm wondering if we couldn't do better in the > case where we're using NFSv4 by using share deny modes (which are > enforced by the server). The problem is that there appears to be > nothing in swapon() that tells the filesystem this is an open of a > swap file... Yeah, it would be nice for completeness to prevent one client from truncating another client's swapfile. However, I'd hope that anyone using swap-over-NFS on a shared NFS mount would take the necessary precautions in terms of permissions, etc. to prevent someone from doing that. Also, since the failure mode of truncating an NFS swapfile is a corrupt swapfile rather than a corrupt filesystem (like on a local filesystem), it's probably okay to just deal with the low-hanging fruit for now. Thanks! > > Cheers > Trond > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com -- Omar