From patchwork Mon Aug 14 08:18:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ernesto_A=2E_Fern=C3=A1ndez?= X-Patchwork-Id: 9898303 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 425AD602D9 for ; Mon, 14 Aug 2017 08:18:18 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 26E91285A5 for ; Mon, 14 Aug 2017 08:18:18 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1B885285A8; Mon, 14 Aug 2017 08:18:18 +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 9527F285A5 for ; Mon, 14 Aug 2017 08:18:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752203AbdHNISQ (ORCPT ); Mon, 14 Aug 2017 04:18:16 -0400 Received: from mail-qk0-f196.google.com ([209.85.220.196]:33522 "EHLO mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751471AbdHNISP (ORCPT ); Mon, 14 Aug 2017 04:18:15 -0400 Received: by mail-qk0-f196.google.com with SMTP id d145so8755640qkc.0 for ; Mon, 14 Aug 2017 01:18:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:mime-version:content-disposition :content-transfer-encoding; bh=OFD1gtGAHL/yuRaae33Vk5KAC9xMgWerkY3O77codt4=; b=lIrwVld7jw8eMq1AQypq+tfnXcz1JH0DXdLAUxk6lXOs0TzpmRl5vQJ3l5f6VAHeAQ L0sVK0Sc7jZPvSgmgCUShBagQA5suh51AXJlsyqHrF8qSudOdbg0rjkM/qJYadoLfLTJ z/lHlQMuvCmtfrCULeCiGkuUecpJyKsSU5D0c2BWPjgQTchFICviU83dhR1F4Y2Z1kJY 7aY+8KatltlJG7Lg+9VR01YUcRi26rluH2GDWPRvbaZlTRfhvUJhA1cGZBnJK4T0Ql6M CQ+heNCjhoJht6eTzwUTBP5mohzhE4UMwz3AvIXbEHiiZdWkHYeNyd25ATdFLyP9jZ+Z DlYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-disposition:content-transfer-encoding; bh=OFD1gtGAHL/yuRaae33Vk5KAC9xMgWerkY3O77codt4=; b=FEBuG7/T36Hc+w3OroKSHq6+Qg73VBYXY+0CVhg//EseQmFJVuaC8EtMLRQCJj7ksJ Un+9kdiFNuA5+1AQF+G0z4IT6R9hHULuMHsVHtcV+b3UtkmSuEaYrXQ7AlpWsaWb/49O K6zfA1vD4ZSzmcNLlPrVcoOdQIEoIoUU6w8Ez41QxWtGsBAT7XkIVChGGneQHdnCPRyT ZUlAtLqALncdzipjsPgxqA/P3ZanzVSqjBtRFZBTPrMwB9DNCkIPlGrUjlSAimcwOIUx V6roDsvz1h3lg+14ckg1U1+np2kZGYeSLMOGueG4JIVt7LgwpRVXOayni37z+0vf1Y+r ATfg== X-Gm-Message-State: AHYfb5jdBUFhgaBQYzMNWtz4yUDwj50l0V/fcF5hA53uG92fOQ+/s11r gOHemTLohLfqyJYP X-Received: by 10.55.133.195 with SMTP id h186mr28981738qkd.21.1502698694863; Mon, 14 Aug 2017 01:18:14 -0700 (PDT) Received: from debian.home ([181.47.56.115]) by smtp.gmail.com with ESMTPSA id j19sm4977702qta.80.2017.08.14.01.18.13 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Aug 2017 01:18:14 -0700 (PDT) Date: Mon, 14 Aug 2017 05:18:10 -0300 From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= To: linux-xfs@vger.kernel.org Cc: "Darrick J. Wong" , Zorro Lang , Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= Subject: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails Message-ID: <20170814081806.GA3924@debian.home> MIME-Version: 1.0 Content-Disposition: inline Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP When changing a file's ACL mask, xfs_set_acl() will first set the group bits of i_mode to the value of the mask, and only then set the actual extended attribute representing the new ACL. If the second part fails (due to lack of space, for example) and the file had no ACL attribute to begin with, the system will from now on assume that the mask permission bits are actual group permission bits, potentially granting access to the wrong users. To prevent this, perform the mode update and the ACL update as part of the same transaction, and restore the original mode to the vfs inode in case of failure. This requires a change in how extended attributes are deleted: commit the transaction even if there was no attribute to remove, to log the standard inode fields. Signed-off-by: Ernesto A. Fernández --- This issue is covered by generic/449 in xfstests. Several other filesystems are affected by this, some of them have already applied patches: - 397e434 ext4: preserve i_mode if __ext4_set_acl() fails - f070e5a jfs: preserve i_mode if __jfs_set_acl() fails - fcea8ae reiserfs: preserve i_mode if __reiserfs_set_acl() fails - fe26569 ext2: preserve i_mode if ext2_set_acl() fails The test won't exactly pass after this patch is applied. It will fail differently, this time claiming that the filesystem is inconsistent. This probably has something to do with setting too many extended attributes, as it goes away if you change the attribute size in the test from 1k to 64k. fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++-- fs/xfs/xfs_acl.c | 39 ++++++++++++++------------------------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index de7b9bd..c3193e1 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -395,6 +395,7 @@ xfs_attr_remove( struct xfs_defer_ops dfops; xfs_fsblock_t firstblock; int error; + int noattr = 0; XFS_STATS_INC(mp, xs_attr_remove); @@ -438,7 +439,12 @@ xfs_attr_remove( xfs_trans_ijoin(args.trans, dp, 0); if (!xfs_inode_hasattr(dp)) { - error = -ENOATTR; + /* + * Commit even if there is no attr to remove, because when + * setting ACLs the caller may be changing the mode in the + * same transaction. + */ + noattr = 1; } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { ASSERT(dp->i_afp->if_flags & XFS_IFINLINE); error = xfs_attr_shortform_remove(&args); @@ -458,7 +464,7 @@ xfs_attr_remove( if (mp->m_flags & XFS_MOUNT_WSYNC) xfs_trans_set_sync(args.trans); - if ((flags & ATTR_KERNOTIME) == 0) + if ((flags & ATTR_KERNOTIME) == 0 && !noattr) xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG); /* @@ -468,6 +474,9 @@ xfs_attr_remove( error = xfs_trans_commit(args.trans); xfs_iunlock(dp, XFS_ILOCK_EXCL); + if (!error && noattr) + error = -ENOATTR; + return error; out: diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c index 7034e17..1fe4363 100644 --- a/fs/xfs/xfs_acl.c +++ b/fs/xfs/xfs_acl.c @@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) return error; } -static int -xfs_set_mode(struct inode *inode, umode_t mode) -{ - int error = 0; - - if (mode != inode->i_mode) { - struct iattr iattr; - - iattr.ia_valid = ATTR_MODE | ATTR_CTIME; - iattr.ia_mode = mode; - iattr.ia_ctime = current_time(inode); - - error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL); - } - - return error; -} - int xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) { int error = 0; + int mode_changed = 0; + umode_t old_mode = inode->i_mode; + struct timespec old_ctime = inode->i_ctime; if (!acl) goto set_acl; @@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) return error; if (type == ACL_TYPE_ACCESS) { - umode_t mode; - - error = posix_acl_update_mode(inode, &mode, &acl); - if (error) - return error; - error = xfs_set_mode(inode, mode); + error = posix_acl_update_mode(inode, &inode->i_mode, &acl); if (error) return error; + inode->i_ctime = current_time(inode); + XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg); + mode_changed = 1; } set_acl: - return __xfs_set_acl(inode, acl, type); + error = __xfs_set_acl(inode, acl, type); + if (error && mode_changed) { + inode->i_mode = old_mode; + inode->i_ctime = old_ctime; + XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg); + } + return error; }