From patchwork Fri Feb 22 18:02:06 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 2176801 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 4D5B8DFABD for ; Fri, 22 Feb 2013 18:02:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758196Ab3BVSCI (ORCPT ); Fri, 22 Feb 2013 13:02:08 -0500 Received: from mx12.netapp.com ([216.240.18.77]:18977 "EHLO mx12.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758134Ab3BVSCI (ORCPT ); Fri, 22 Feb 2013 13:02:08 -0500 X-IronPort-AV: E=Sophos;i="4.84,717,1355126400"; d="scan'208,223";a="25246186" Received: from smtp2.corp.netapp.com ([10.57.159.114]) by mx12-out.netapp.com with ESMTP; 22 Feb 2013 10:02:07 -0800 Received: from vmwexceht04-prd.hq.netapp.com (vmwexceht04-prd.hq.netapp.com [10.106.77.34]) by smtp2.corp.netapp.com (8.13.1/8.13.1/NTAP-1.6) with ESMTP id r1MI27WL015310; Fri, 22 Feb 2013 10:02:07 -0800 (PST) Received: from SACEXCMBX04-PRD.hq.netapp.com ([169.254.6.77]) by vmwexceht04-prd.hq.netapp.com ([10.106.77.34]) with mapi id 14.02.0328.009; Fri, 22 Feb 2013 10:02:07 -0800 From: "Myklebust, Trond" To: Dave Wysochanski CC: "linux-nfs@vger.kernel.org" , "jlayton@redhat.com" Subject: Re: [PATCH] nfs: Don't allow NFS silly-renamed files to be deleted, no signal Thread-Topic: [PATCH] nfs: Don't allow NFS silly-renamed files to be deleted, no signal Thread-Index: AQHODqmJUAjyvPXNy0aBIL+ExrG+uZiGtj+A Date: Fri, 22 Feb 2013 18:02:06 +0000 Message-ID: <4FA345DA4F4AE44899BD2B03EEEC2FA9235DC4AF@SACEXCMBX04-PRD.hq.netapp.com> References: <1361282391-6578-1-git-send-email-dwysocha@redhat.com> In-Reply-To: <1361282391-6578-1-git-send-email-dwysocha@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: x-originating-ip: [10.104.60.115] MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org On Tue, 2013-02-19 at 08:59 -0500, Dave Wysochanski wrote: > Commit 73ca100 broke the code that prevents the client from deleting > a silly renamed dentry. This affected "delete on last close" > semantics as after that commit, nothing prevented removal of > silly-renamed files. As a result, a process holding a file open > could easily get an ESTALE on the file in a directory where some > other process issued 'rm -rf some_dir_containing_the_file' twice. > Before the commit, any attempt at unlinking silly renamed files would > fail inside may_delete() with -EBUSY because of the > DCACHE_NFSFS_RENAMED flag. The following testcase demonstrates > the problem: > tail -f /nfsmnt/dir/file & > rm -rf /nfsmnt/dir > rm -rf /nfsmnt/dir > # second removal does not fail, 'tail' process receives ESTALE Hi Dave, I'm not sure I understand why we must do the dropping/moving of the dentries inside nfs_async_rename_done. Why isn't something like the attached patch sufficient? Cheers, Trond Acked-by: Jeff Layton From 90769817a805292646623cedd9d5455d31b63bc2 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Fri, 22 Feb 2013 12:53:43 -0500 Subject: [PATCH] NFS: Don't allow NFS silly-renamed files to be deleted, no signal Commit 73ca100 broke the code that prevents the client from deleting a silly renamed dentry. This affected "delete on last close" semantics as after that commit, nothing prevented removal of silly-renamed files. As a result, a process holding a file open could easily get an ESTALE on the file in a directory where some other process issued 'rm -rf some_dir_containing_the_file' twice. Before the commit, any attempt at unlinking silly renamed files would fail inside may_delete() with -EBUSY because of the DCACHE_NFSFS_RENAMED flag. The following testcase demonstrates the problem: tail -f /nfsmnt/dir/file & rm -rf /nfsmnt/dir rm -rf /nfsmnt/dir # second removal does not fail, 'tail' process receives ESTALE The problem with the above commit is that it unhashes the old and new dentries from the lookup path, even in the normal case when a signal is not encountered and it would have been safe to call d_move. Unfortunately the old dentry has the special DCACHE_NFSFS_RENAMED flag set on it. Unhashing has the side-effect that future lookups call d_alloc(), allocating a new dentry without the special flag for any silly-renamed files. As a result, subsequent calls to unlink silly renamed files do not fail but allow the removal to go through. This will result in ESTALE errors for any other process doing operations on the file. To fix this, go back to using d_move on success. For the signal case, it's unclear what we may safely do beyond d_drop. Reported-by: Dave Wysochanski Signed-off-by: Trond Myklebust Cc: stable@vger.kernel.org --- fs/nfs/unlink.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c index d26a32f..6a8368e 100644 --- a/fs/nfs/unlink.c +++ b/fs/nfs/unlink.c @@ -335,20 +335,14 @@ static void nfs_async_rename_done(struct rpc_task *task, void *calldata) struct inode *old_dir = data->old_dir; struct inode *new_dir = data->new_dir; struct dentry *old_dentry = data->old_dentry; - struct dentry *new_dentry = data->new_dentry; if (!NFS_PROTO(old_dir)->rename_done(task, old_dir, new_dir)) { rpc_restart_call_prepare(task); return; } - if (task->tk_status != 0) { + if (task->tk_status != 0) nfs_cancel_async_unlink(old_dentry); - return; - } - - d_drop(old_dentry); - d_drop(new_dentry); } /** @@ -549,6 +543,15 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry) error = rpc_wait_for_completion_task(task); if (error == 0) error = task->tk_status; + switch (error) { + case 0: + nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); + d_move(dentry, sdentry); + break; + case -ERESTARTSYS: + d_drop(dentry); + d_drop(sdentry); + } rpc_put_task(task); out_dput: dput(sdentry); -- 1.8.1.2