Message ID | 20221201104125.919483-3-roberto.sassu@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp188263wrr; Thu, 1 Dec 2022 02:44:44 -0800 (PST) X-Google-Smtp-Source: AA0mqf7xENNkMQwL9CZ6A9un1JE19PZtREqiQY8H80yo4ujWlpvytSBbM8hX6OPuJ++nB5Nu8b5U X-Received: by 2002:a17:906:ae99:b0:7c0:82d8:243a with SMTP id md25-20020a170906ae9900b007c082d8243amr11371150ejb.230.1669891484649; Thu, 01 Dec 2022 02:44:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669891484; cv=none; d=google.com; s=arc-20160816; b=cHgRzASKjYZpZyAqh8CBp79wXp8Ig8hVv6QTwEUjNujJxqeiI8a/1qofGElgmATtmZ ozKgAyoK60JX2MXbU6QXdfI8FhO7JhwCGysf6OazAgQHDfKLGReYD8VtRIYjJXhHHoX2 RMDva9tnjlxcg+22z2aLJpiUBJeveuLGxyof7+5XFVMAeyz7H6YbDk1R6uf6o5K+ZoaR JjXD8idoJcqlM/iy6fb2hdGVTy5ud81R1OPoyRPNxx5h6TsYI8aS/UNRlUCapMsMTenw ACaX0o0rRTuOPE0ovoN4Xm/+lUasXFIoyJUwgjm9HSVsiHgI7VPftBk3mm2JBqhYDFoD mKRQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=FSxpc9b2UF44yw/1SEHpHJQshcYp8E45t37luHjJGd8=; b=V9pVYOhVq1CMXYVVXuQdLjobOA4Z2tS90YjxlaBV+K/9fkusXldnsVbRtnlB+Oo19R yYhLazCZzlGB1slmTtIwpsprMdg6hscB0n9fQZKlWiCNxios8JKIzD2FxkViBDFYuFqq 6gBPO34+husPFHLkcOUCUUUh3kXOCOXLzpqFl7W+z6PlS6gAJMfLPAIK5b5OAx2Ck8Ti ic1cK6OtElntKluZXfXCv3CK9ra+dV0bWQlnlp6e1A/RqaVziOGSIY/hmywvHQAo8Zl9 2BzNs63WPdRc6a8LZNVgTa05lzgM4aug5Re5agktvAorp7K2zPT5bFCItft1+u2vWII0 +TeQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id hc11-20020a170907168b00b0078db3ce1e59si4317495ejc.38.2022.12.01.02.44.21; Thu, 01 Dec 2022 02:44:44 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231158AbiLAKni (ORCPT <rfc822;heyuhang3455@gmail.com> + 99 others); Thu, 1 Dec 2022 05:43:38 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50522 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230146AbiLAKnD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 1 Dec 2022 05:43:03 -0500 Received: from frasgout12.his.huawei.com (frasgout12.his.huawei.com [14.137.139.154]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 70DAB286D9; Thu, 1 Dec 2022 02:42:44 -0800 (PST) Received: from mail02.huawei.com (unknown [172.18.147.227]) by frasgout12.his.huawei.com (SkyGuard) with ESMTP id 4NNCCv1ZgLz9xFY9; Thu, 1 Dec 2022 18:36:11 +0800 (CST) Received: from huaweicloud.com (unknown [10.204.63.22]) by APP1 (Coremail) with SMTP id LxC2BwCHcm_phIhjrxuvAA--.49496S4; Thu, 01 Dec 2022 11:42:16 +0100 (CET) From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: mark@fasheh.com, jlbec@evilplan.org, joseph.qi@linux.alibaba.com, zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, stephen.smalley.work@gmail.com, eparis@parisplace.org, casey@schaufler-ca.com Cc: ocfs2-devel@oss.oracle.com, reiserfs-devel@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, linux-kernel@vger.kernel.org, keescook@chromium.org, nicolas.bouchinet@clip-os.org, Roberto Sassu <roberto.sassu@huawei.com> Subject: [PATCH v7 2/6] ocfs2: Switch to security_inode_init_security() Date: Thu, 1 Dec 2022 11:41:21 +0100 Message-Id: <20221201104125.919483-3-roberto.sassu@huaweicloud.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20221201104125.919483-1-roberto.sassu@huaweicloud.com> References: <20221201104125.919483-1-roberto.sassu@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: LxC2BwCHcm_phIhjrxuvAA--.49496S4 X-Coremail-Antispam: 1UD129KBjvJXoWxtw4kKF1UJw1rCrW5Aw1fZwb_yoW7Xr1Upa 1ftFnxtr4rJFyUWryftr45ua1S9rWrGrZrGrs3G34DZFn8Cr1ftry0yr15ua45XrWDJFyk tr4Fkrsxuan8J37anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUB2b4IE77IF4wAFF20E14v26rWj6s0DM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28IrcIa0xkI8VA2jI8067AKxVWUXw A2048vs2IY020Ec7CjxVAFwI0_Xr0E3s1l8cAvFVAK0II2c7xJM28CjxkF64kEwVA0rcxS w2x7M28EF7xvwVC0I7IYx2IY67AKxVWUJVWUCwA2z4x0Y4vE2Ix0cI8IcVCY1x0267AKxV WxJVW8Jr1l84ACjcxK6I8E87Iv67AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_ Gr1j6F4UJwAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG6I80ew Av7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY 6r1j6r4UM4x0Y48IcxkI7VAKI48JM4IIrI8v6xkF7I0E8cxan2IY04v7MxAIw28IcxkI7V AKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr4lx2IqxVCj r7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVW8ZVWrXwCIc40Y0x0EwIxGrwCI42IY6x IIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x0267AKxVWxJVW8Jr1lIxAIcVCF 04k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIEc7 CjxVAFwI0_Gr1j6F4UJbIYCTnIWIevJa73UjIFyTuYvjxUFa9-UUUUU X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAgANBF1jj4IjiwAAsQ X-CFilter-Loop: Reflected X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1751008133133117533?= X-GMAIL-MSGID: =?utf-8?q?1751008133133117533?= |
Series |
evm: Do HMAC of multiple per LSM xattrs for new inodes
|
|
Commit Message
Roberto Sassu
Dec. 1, 2022, 10:41 a.m. UTC
From: Roberto Sassu <roberto.sassu@huawei.com> In preparation for removing security_old_inode_init_security(), switch to security_inode_init_security(). Extend the existing ocfs2_initxattrs() to take the ocfs2_security_xattr_info structure from fs_info, and populate the name/value/len triple with the first xattr provided by LSMs. As fs_info was not used before, ocfs2_initxattrs() can now handle the case of replicating the behavior of security_old_inode_init_security(), i.e. just obtaining the xattr, in addition to setting all xattrs provided by LSMs. Supporting multiple xattrs is not currently supported where security_old_inode_init_security() was called (mknod, symlink), as it requires non-trivial changes that can be done at a later time. Like for reiserfs, even if EVM is invoked, it will not provide an xattr (if it is not the first to set it, its xattr will be discarded; if it is the first, it does not have xattrs to calculate the HMAC on). Finally, modify the handling of the return value from ocfs2_init_security_get(). As security_inode_init_security() does not return -EOPNOTSUPP, remove this case and directly handle the error if the return value is not zero. However, the previous case of receiving -EOPNOTSUPP should be still taken into account, as security_inode_init_security() could return zero without setting xattrs and ocfs2 would consider it as if the xattr was set. Instead, if security_inode_init_security() returned zero, look at the xattr if it was set, and behave accordingly, i.e. set si->enable to zero to notify to the functions following ocfs2_init_security_get() that the xattr is not available (same as if security_old_inode_init_security() returned -EOPNOTSUPP). Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> --- fs/ocfs2/namei.c | 18 ++++++------------ fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 16 deletions(-)
Comments
On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > In preparation for removing security_old_inode_init_security(), switch to > security_inode_init_security(). > > Extend the existing ocfs2_initxattrs() to take the > ocfs2_security_xattr_info structure from fs_info, and populate the > name/value/len triple with the first xattr provided by LSMs. Hi Mark, Joel, Joseph some time ago I sent this patch set to switch to the newer function security_inode_init_security(). Almost all the other parts of this patch set have been reviewed, and the patch set itself should be ready to be merged. I kindly ask if you could have a look at this patch and give your Reviewed-by, so that Paul could take the patch set. Thanks a lot! Roberto > As fs_info was not used before, ocfs2_initxattrs() can now handle the case > of replicating the behavior of security_old_inode_init_security(), i.e. > just obtaining the xattr, in addition to setting all xattrs provided by > LSMs. > > Supporting multiple xattrs is not currently supported where > security_old_inode_init_security() was called (mknod, symlink), as it > requires non-trivial changes that can be done at a later time. Like for > reiserfs, even if EVM is invoked, it will not provide an xattr (if it is > not the first to set it, its xattr will be discarded; if it is the first, > it does not have xattrs to calculate the HMAC on). > > Finally, modify the handling of the return value from > ocfs2_init_security_get(). As security_inode_init_security() does not > return -EOPNOTSUPP, remove this case and directly handle the error if the > return value is not zero. > > However, the previous case of receiving -EOPNOTSUPP should be still > taken into account, as security_inode_init_security() could return zero > without setting xattrs and ocfs2 would consider it as if the xattr was set. > > Instead, if security_inode_init_security() returned zero, look at the xattr > if it was set, and behave accordingly, i.e. set si->enable to zero to > notify to the functions following ocfs2_init_security_get() that the xattr > is not available (same as if security_old_inode_init_security() returned > -EOPNOTSUPP). > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/ocfs2/namei.c | 18 ++++++------------ > fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- > 2 files changed, 32 insertions(+), 16 deletions(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index 05f32989bad6..55fba81cd2d1 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > int want_meta = 0; > int xattr_credits = 0; > struct ocfs2_security_xattr_info si = { > + .name = NULL, > .enable = 1, > }; > int did_quota_inode = 0; > @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > /* get security xattr */ > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > if (status) { > - if (status == -EOPNOTSUPP) > - si.enable = 0; > - else { > - mlog_errno(status); > - goto leave; > - } > + mlog_errno(status); > + goto leave; > } > > /* calculate meta data/clusters for setting security and acl xattr */ > @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > int want_clusters = 0; > int xattr_credits = 0; > struct ocfs2_security_xattr_info si = { > + .name = NULL, > .enable = 1, > }; > int did_quota = 0, did_quota_inode = 0; > @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > /* get security xattr */ > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > if (status) { > - if (status == -EOPNOTSUPP) > - si.enable = 0; > - else { > - mlog_errno(status); > - goto bail; > - } > + mlog_errno(status); > + goto bail; > } > > /* calculate meta data/clusters for setting security xattr */ > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > index 95d0611c5fc7..55699c573541 100644 > --- a/fs/ocfs2/xattr.c > +++ b/fs/ocfs2/xattr.c > @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, > static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, > void *fs_info) > { > + struct ocfs2_security_xattr_info *si = fs_info; > const struct xattr *xattr; > int err = 0; > > + if (si) { > + si->value = kmemdup(xattr_array->value, xattr_array->value_len, > + GFP_KERNEL); > + if (!si->value) > + return -ENOMEM; > + > + si->name = xattr_array->name; > + si->value_len = xattr_array->value_len; > + return 0; > + } > + > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, > xattr->name, xattr->value, > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > const struct qstr *qstr, > struct ocfs2_security_xattr_info *si) > { > + int ret; > + > /* check whether ocfs2 support feature xattr */ > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > return -EOPNOTSUPP; > - if (si) > - return security_old_inode_init_security(inode, dir, qstr, > - &si->name, &si->value, > - &si->value_len); > + if (si) { > + ret = security_inode_init_security(inode, dir, qstr, > + &ocfs2_initxattrs, si); > + /* > + * security_inode_init_security() does not return -EOPNOTSUPP, > + * we have to check the xattr ourselves. > + */ > + if (!ret && !si->name) > + si->enable = 0; > + > + return ret; > + } > > return security_inode_init_security(inode, dir, qstr, > &ocfs2_initxattrs, NULL);
On Tue, Jan 10, 2023 at 3:56 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > In preparation for removing security_old_inode_init_security(), switch to > > security_inode_init_security(). > > > > Extend the existing ocfs2_initxattrs() to take the > > ocfs2_security_xattr_info structure from fs_info, and populate the > > name/value/len triple with the first xattr provided by LSMs. > > Hi Mark, Joel, Joseph > > some time ago I sent this patch set to switch to the newer > function security_inode_init_security(). Almost all the other parts of > this patch set have been reviewed, and the patch set itself should be > ready to be merged. > > I kindly ask if you could have a look at this patch and give your > Reviewed-by, so that Paul could take the patch set. I've been pushing to clean up some of the LSM interfaces to try and simplify things and remove as many special cases as possible, Roberto's work in this patchset is part of that. I would really appreciate it if the vfs/ocfs2 folks could give patch 2/6 a quick look to make sure you are okay with the changes. I realize that the various end-of-year holidays tend to slow things down a bit, but this patchset has been on the lists for over a month now; if I don't hear anything in the next week or two I'll assume you folks are okay with these patches ...
On Thu, 2023-01-12 at 12:21 -0500, Paul Moore wrote: > On Tue, Jan 10, 2023 at 3:56 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > In preparation for removing security_old_inode_init_security(), switch to > > > security_inode_init_security(). > > > > > > Extend the existing ocfs2_initxattrs() to take the > > > ocfs2_security_xattr_info structure from fs_info, and populate the > > > name/value/len triple with the first xattr provided by LSMs. > > > > Hi Mark, Joel, Joseph > > > > some time ago I sent this patch set to switch to the newer > > function security_inode_init_security(). Almost all the other parts of > > this patch set have been reviewed, and the patch set itself should be > > ready to be merged. > > > > I kindly ask if you could have a look at this patch and give your > > Reviewed-by, so that Paul could take the patch set. > > I've been pushing to clean up some of the LSM interfaces to try and > simplify things and remove as many special cases as possible, > Roberto's work in this patchset is part of that. I would really > appreciate it if the vfs/ocfs2 folks could give patch 2/6 a quick look > to make sure you are okay with the changes. > > I realize that the various end-of-year holidays tend to slow things > down a bit, but this patchset has been on the lists for over a month > now; if I don't hear anything in the next week or two I'll assume you > folks are okay with these patches ... Hi Paul is this patch set going to land in 6.3? Thanks Roberto
On Wed, Feb 8, 2023 at 9:33 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > On Thu, 2023-01-12 at 12:21 -0500, Paul Moore wrote: > > On Tue, Jan 10, 2023 at 3:56 AM Roberto Sassu > > <roberto.sassu@huaweicloud.com> wrote: > > > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > In preparation for removing security_old_inode_init_security(), switch to > > > > security_inode_init_security(). > > > > > > > > Extend the existing ocfs2_initxattrs() to take the > > > > ocfs2_security_xattr_info structure from fs_info, and populate the > > > > name/value/len triple with the first xattr provided by LSMs. > > > > > > Hi Mark, Joel, Joseph > > > > > > some time ago I sent this patch set to switch to the newer > > > function security_inode_init_security(). Almost all the other parts of > > > this patch set have been reviewed, and the patch set itself should be > > > ready to be merged. > > > > > > I kindly ask if you could have a look at this patch and give your > > > Reviewed-by, so that Paul could take the patch set. > > > > I've been pushing to clean up some of the LSM interfaces to try and > > simplify things and remove as many special cases as possible, > > Roberto's work in this patchset is part of that. I would really > > appreciate it if the vfs/ocfs2 folks could give patch 2/6 a quick look > > to make sure you are okay with the changes. > > > > I realize that the various end-of-year holidays tend to slow things > > down a bit, but this patchset has been on the lists for over a month > > now; if I don't hear anything in the next week or two I'll assume you > > folks are okay with these patches ... > > Hi Paul > > is this patch set going to land in 6.3? Hi Roberto, I had really hoped the vfs/ocfs2 folks would have commented on this by now, but it's been over two months now with no comments that I can see so I think we have to do it ourselves via the LSM tree. It's obviously too late for the upcoming merge window, so no v6.3, but I think we can merge it *after* the upcoming merge window closes, assuming we get ACKs from Mimi on the EVM bits (I still need to review it too, but I'm not expecting anything too bad).
On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > In preparation for removing security_old_inode_init_security(), switch to > security_inode_init_security(). > > Extend the existing ocfs2_initxattrs() to take the > ocfs2_security_xattr_info structure from fs_info, and populate the > name/value/len triple with the first xattr provided by LSMs. > > As fs_info was not used before, ocfs2_initxattrs() can now handle the case > of replicating the behavior of security_old_inode_init_security(), i.e. > just obtaining the xattr, in addition to setting all xattrs provided by > LSMs. > > Supporting multiple xattrs is not currently supported where > security_old_inode_init_security() was called (mknod, symlink), as it > requires non-trivial changes that can be done at a later time. Like for > reiserfs, even if EVM is invoked, it will not provide an xattr (if it is > not the first to set it, its xattr will be discarded; if it is the first, > it does not have xattrs to calculate the HMAC on). > > Finally, modify the handling of the return value from > ocfs2_init_security_get(). As security_inode_init_security() does not > return -EOPNOTSUPP, remove this case and directly handle the error if the > return value is not zero. > > However, the previous case of receiving -EOPNOTSUPP should be still > taken into account, as security_inode_init_security() could return zero > without setting xattrs and ocfs2 would consider it as if the xattr was set. > > Instead, if security_inode_init_security() returned zero, look at the xattr > if it was set, and behave accordingly, i.e. set si->enable to zero to > notify to the functions following ocfs2_init_security_get() that the xattr > is not available (same as if security_old_inode_init_security() returned > -EOPNOTSUPP). > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> My previous review missed a couple of concerns. > --- > fs/ocfs2/namei.c | 18 ++++++------------ > fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- > 2 files changed, 32 insertions(+), 16 deletions(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index 05f32989bad6..55fba81cd2d1 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > int want_meta = 0; > int xattr_credits = 0; > struct ocfs2_security_xattr_info si = { > + .name = NULL, > .enable = 1, > }; > int did_quota_inode = 0; > @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > /* get security xattr */ > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > if (status) { > - if (status == -EOPNOTSUPP) > - si.enable = 0; > - else { > - mlog_errno(status); > - goto leave; > - } Although security_inode_init_security() does not return -EOPNOTSUPP, ocfs2_init_security_get() could. Refer to commit 8154da3d2114 ("ocfs2: Add incompatible flag for extended attribute"). It was added as a temporary solution back in 2008, so it is highly unlikely that it is still needed. > + mlog_errno(status); > + goto leave; Without the -EOPNOTSUPP test, ocfs2_mknod() would not create the inode; and similarly ocfs2_symlink(), below, would not create the symlink. It would be safer not to remove the -EOPNOTSUPP test. > } > > /* calculate meta data/clusters for setting security and acl xattr */ > @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > int want_clusters = 0; > int xattr_credits = 0; > struct ocfs2_security_xattr_info si = { > + .name = NULL, > .enable = 1, > }; > int did_quota = 0, did_quota_inode = 0; > @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > /* get security xattr */ > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > if (status) { > - if (status == -EOPNOTSUPP) > - si.enable = 0; > - else { > - mlog_errno(status); > - goto bail; > - } > + mlog_errno(status); > + goto bail; > } > > /* calculate meta data/clusters for setting security xattr */ > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > index 95d0611c5fc7..55699c573541 100644 > --- a/fs/ocfs2/xattr.c > +++ b/fs/ocfs2/xattr.c > @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, > static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, > void *fs_info) > { > + struct ocfs2_security_xattr_info *si = fs_info; > const struct xattr *xattr; > int err = 0; > > + if (si) { > + si->value = kmemdup(xattr_array->value, xattr_array->value_len, > + GFP_KERNEL); > + if (!si->value) > + return -ENOMEM; > + > + si->name = xattr_array->name; > + si->value_len = xattr_array->value_len; > + return 0; > + } > + > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, > xattr->name, xattr->value, > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > const struct qstr *qstr, > struct ocfs2_security_xattr_info *si) > { > + int ret; > + > /* check whether ocfs2 support feature xattr */ > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > return -EOPNOTSUPP; > - if (si) > - return security_old_inode_init_security(inode, dir, qstr, > - &si->name, &si->value, > - &si->value_len); > + if (si) { > + ret = security_inode_init_security(inode, dir, qstr, > + &ocfs2_initxattrs, si); The "if (unlikely(IS_PRIVATE(inode))" test exists in both security_old_inode_init_security() and security_inode_init_security(), but return different values. In the former case, it returns -EOPNOTSUPP. In the latter case, it returns 0. The question is whether or not we need to be concerned about private inodes on ocfs2. If private inodes on ocfs2 are possible, then ocsf2_mknod() or ocfs2_symlink() would fail to create the inode or symlink. > + /* > + * security_inode_init_security() does not return -EOPNOTSUPP, > + * we have to check the xattr ourselves. > + */ > + if (!ret && !si->name) > + si->enable = 0; > + > + return ret; > + } > > return security_inode_init_security(inode, dir, qstr, > &ocfs2_initxattrs, NULL);
On Fri, 2023-02-17 at 14:51 -0500, Mimi Zohar wrote: > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > In preparation for removing security_old_inode_init_security(), switch to > > security_inode_init_security(). > > > > Extend the existing ocfs2_initxattrs() to take the > > ocfs2_security_xattr_info structure from fs_info, and populate the > > name/value/len triple with the first xattr provided by LSMs. > > > > As fs_info was not used before, ocfs2_initxattrs() can now handle the case > > of replicating the behavior of security_old_inode_init_security(), i.e. > > just obtaining the xattr, in addition to setting all xattrs provided by > > LSMs. > > > > Supporting multiple xattrs is not currently supported where > > security_old_inode_init_security() was called (mknod, symlink), as it > > requires non-trivial changes that can be done at a later time. Like for > > reiserfs, even if EVM is invoked, it will not provide an xattr (if it is > > not the first to set it, its xattr will be discarded; if it is the first, > > it does not have xattrs to calculate the HMAC on). > > > > Finally, modify the handling of the return value from > > ocfs2_init_security_get(). As security_inode_init_security() does not > > return -EOPNOTSUPP, remove this case and directly handle the error if the > > return value is not zero. > > > > However, the previous case of receiving -EOPNOTSUPP should be still > > taken into account, as security_inode_init_security() could return zero > > without setting xattrs and ocfs2 would consider it as if the xattr was set. > > > > Instead, if security_inode_init_security() returned zero, look at the xattr > > if it was set, and behave accordingly, i.e. set si->enable to zero to > > notify to the functions following ocfs2_init_security_get() that the xattr > > is not available (same as if security_old_inode_init_security() returned > > -EOPNOTSUPP). > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > > My previous review missed a couple of concerns. > > > --- > > fs/ocfs2/namei.c | 18 ++++++------------ > > fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- > > 2 files changed, 32 insertions(+), 16 deletions(-) > > > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > > index 05f32989bad6..55fba81cd2d1 100644 > > --- a/fs/ocfs2/namei.c > > +++ b/fs/ocfs2/namei.c > > @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > int want_meta = 0; > > int xattr_credits = 0; > > struct ocfs2_security_xattr_info si = { > > + .name = NULL, > > .enable = 1, > > }; > > int did_quota_inode = 0; > > @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > /* get security xattr */ > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > if (status) { > > - if (status == -EOPNOTSUPP) > > - si.enable = 0; > > - else { > > - mlog_errno(status); > > - goto leave; > > - } > > Although security_inode_init_security() does not return -EOPNOTSUPP, > ocfs2_init_security_get() could. Refer to commit 8154da3d2114 ("ocfs2: > Add incompatible flag for extended attribute"). It was added as a > temporary solution back in 2008, so it is highly unlikely that it is > still needed. > > > + mlog_errno(status); > > + goto leave; > > Without the -EOPNOTSUPP test, ocfs2_mknod() would not create the inode; > and similarly ocfs2_symlink(), below, would not create the symlink. It > would be safer not to remove the -EOPNOTSUPP test. > > > } > > > > /* calculate meta data/clusters for setting security and acl xattr */ > > @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > int want_clusters = 0; > > int xattr_credits = 0; > > struct ocfs2_security_xattr_info si = { > > + .name = NULL, > > .enable = 1, > > }; > > int did_quota = 0, did_quota_inode = 0; > > @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > /* get security xattr */ > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > if (status) { > > - if (status == -EOPNOTSUPP) > > - si.enable = 0; > > - else { > > - mlog_errno(status); > > - goto bail; > > - } > > + mlog_errno(status); > > + goto bail; > > } > > > > /* calculate meta data/clusters for setting security xattr */ > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > index 95d0611c5fc7..55699c573541 100644 > > --- a/fs/ocfs2/xattr.c > > +++ b/fs/ocfs2/xattr.c > > @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, > > static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, > > void *fs_info) > > { > > + struct ocfs2_security_xattr_info *si = fs_info; > > const struct xattr *xattr; > > int err = 0; > > > > + if (si) { > > + si->value = kmemdup(xattr_array->value, xattr_array->value_len, > > + GFP_KERNEL); > > + if (!si->value) > > + return -ENOMEM; > > + > > + si->name = xattr_array->name; > > + si->value_len = xattr_array->value_len; > > + return 0; > > + } > > + > > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > > err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, > > xattr->name, xattr->value, > > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > > const struct qstr *qstr, > > struct ocfs2_security_xattr_info *si) > > { > > + int ret; > > + > > /* check whether ocfs2 support feature xattr */ > > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > > return -EOPNOTSUPP; > > - if (si) > > - return security_old_inode_init_security(inode, dir, qstr, > > - &si->name, &si->value, > > - &si->value_len); > > + if (si) { > > + ret = security_inode_init_security(inode, dir, qstr, > > + &ocfs2_initxattrs, si); > > The "if (unlikely(IS_PRIVATE(inode))" test exists in both > security_old_inode_init_security() and security_inode_init_security(), > but return different values. In the former case, it returns > -EOPNOTSUPP. In the latter case, it returns 0. The question is > whether or not we need to be concerned about private inodes on ocfs2. > If private inodes on ocfs2 are possible, then ocsf2_mknod() or > ocfs2_symlink() would fail to create the inode or symlink. Correction, previously when returning -EOPNOTSUPP for private inodes, xattrs would not be wrriten. By returning 0 without setting si->enable to 0, xattrs will be written. > > > + /* > > + * security_inode_init_security() does not return -EOPNOTSUPP, > > + * we have to check the xattr ourselves. > > + */ > > + if (!ret && !si->name) > > + si->enable = 0; > > + > > + return ret; > > + } > > > > return security_inode_init_security(inode, dir, qstr, > > &ocfs2_initxattrs, NULL); >
On Fri, 2023-02-17 at 14:51 -0500, Mimi Zohar wrote: > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > In preparation for removing security_old_inode_init_security(), switch to > > security_inode_init_security(). > > > > Extend the existing ocfs2_initxattrs() to take the > > ocfs2_security_xattr_info structure from fs_info, and populate the > > name/value/len triple with the first xattr provided by LSMs. > > > > As fs_info was not used before, ocfs2_initxattrs() can now handle the case > > of replicating the behavior of security_old_inode_init_security(), i.e. > > just obtaining the xattr, in addition to setting all xattrs provided by > > LSMs. > > > > Supporting multiple xattrs is not currently supported where > > security_old_inode_init_security() was called (mknod, symlink), as it > > requires non-trivial changes that can be done at a later time. Like for > > reiserfs, even if EVM is invoked, it will not provide an xattr (if it is > > not the first to set it, its xattr will be discarded; if it is the first, > > it does not have xattrs to calculate the HMAC on). > > > > Finally, modify the handling of the return value from > > ocfs2_init_security_get(). As security_inode_init_security() does not > > return -EOPNOTSUPP, remove this case and directly handle the error if the > > return value is not zero. > > > > However, the previous case of receiving -EOPNOTSUPP should be still > > taken into account, as security_inode_init_security() could return zero > > without setting xattrs and ocfs2 would consider it as if the xattr was set. > > > > Instead, if security_inode_init_security() returned zero, look at the xattr > > if it was set, and behave accordingly, i.e. set si->enable to zero to > > notify to the functions following ocfs2_init_security_get() that the xattr > > is not available (same as if security_old_inode_init_security() returned > > -EOPNOTSUPP). > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > > My previous review missed a couple of concerns. > > > --- > > fs/ocfs2/namei.c | 18 ++++++------------ > > fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- > > 2 files changed, 32 insertions(+), 16 deletions(-) > > > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > > index 05f32989bad6..55fba81cd2d1 100644 > > --- a/fs/ocfs2/namei.c > > +++ b/fs/ocfs2/namei.c > > @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > int want_meta = 0; > > int xattr_credits = 0; > > struct ocfs2_security_xattr_info si = { > > + .name = NULL, > > .enable = 1, > > }; > > int did_quota_inode = 0; > > @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > /* get security xattr */ > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > if (status) { > > - if (status == -EOPNOTSUPP) > > - si.enable = 0; > > - else { > > - mlog_errno(status); > > - goto leave; > > - } > > Although security_inode_init_security() does not return -EOPNOTSUPP, > ocfs2_init_security_get() could. Refer to commit 8154da3d2114 ("ocfs2: > Add incompatible flag for extended attribute"). It was added as a > temporary solution back in 2008, so it is highly unlikely that it is > still needed. > > > + mlog_errno(status); > > + goto leave; > > Without the -EOPNOTSUPP test, ocfs2_mknod() would not create the inode; > and similarly ocfs2_symlink(), below, would not create the symlink. It > would be safer not to remove the -EOPNOTSUPP test. You are absolutely right. Will add it back. Thanks Roberto > > } > > > > /* calculate meta data/clusters for setting security and acl xattr */ > > @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > int want_clusters = 0; > > int xattr_credits = 0; > > struct ocfs2_security_xattr_info si = { > > + .name = NULL, > > .enable = 1, > > }; > > int did_quota = 0, did_quota_inode = 0; > > @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > /* get security xattr */ > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > if (status) { > > - if (status == -EOPNOTSUPP) > > - si.enable = 0; > > - else { > > - mlog_errno(status); > > - goto bail; > > - } > > + mlog_errno(status); > > + goto bail; > > } > > > > /* calculate meta data/clusters for setting security xattr */ > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > index 95d0611c5fc7..55699c573541 100644 > > --- a/fs/ocfs2/xattr.c > > +++ b/fs/ocfs2/xattr.c > > @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, > > static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, > > void *fs_info) > > { > > + struct ocfs2_security_xattr_info *si = fs_info; > > const struct xattr *xattr; > > int err = 0; > > > > + if (si) { > > + si->value = kmemdup(xattr_array->value, xattr_array->value_len, > > + GFP_KERNEL); > > + if (!si->value) > > + return -ENOMEM; > > + > > + si->name = xattr_array->name; > > + si->value_len = xattr_array->value_len; > > + return 0; > > + } > > + > > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > > err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, > > xattr->name, xattr->value, > > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > > const struct qstr *qstr, > > struct ocfs2_security_xattr_info *si) > > { > > + int ret; > > + > > /* check whether ocfs2 support feature xattr */ > > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > > return -EOPNOTSUPP; > > - if (si) > > - return security_old_inode_init_security(inode, dir, qstr, > > - &si->name, &si->value, > > - &si->value_len); > > + if (si) { > > + ret = security_inode_init_security(inode, dir, qstr, > > + &ocfs2_initxattrs, si); > > The "if (unlikely(IS_PRIVATE(inode))" test exists in both > security_old_inode_init_security() and security_inode_init_security(), > but return different values. In the former case, it returns > -EOPNOTSUPP. In the latter case, it returns 0. The question is > whether or not we need to be concerned about private inodes on ocfs2. > If private inodes on ocfs2 are possible, then ocsf2_mknod() or > ocfs2_symlink() would fail to create the inode or symlink. > > > + /* > > + * security_inode_init_security() does not return -EOPNOTSUPP, > > + * we have to check the xattr ourselves. > > + */ > > + if (!ret && !si->name) > > + si->enable = 0; > > + > > + return ret; > > + } > > > > return security_inode_init_security(inode, dir, qstr, > > &ocfs2_initxattrs, NULL);
On Fri, 2023-02-17 at 16:30 -0500, Mimi Zohar wrote: > On Fri, 2023-02-17 at 14:51 -0500, Mimi Zohar wrote: > > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > In preparation for removing security_old_inode_init_security(), switch to > > > security_inode_init_security(). > > > > > > Extend the existing ocfs2_initxattrs() to take the > > > ocfs2_security_xattr_info structure from fs_info, and populate the > > > name/value/len triple with the first xattr provided by LSMs. > > > > > > As fs_info was not used before, ocfs2_initxattrs() can now handle the case > > > of replicating the behavior of security_old_inode_init_security(), i.e. > > > just obtaining the xattr, in addition to setting all xattrs provided by > > > LSMs. > > > > > > Supporting multiple xattrs is not currently supported where > > > security_old_inode_init_security() was called (mknod, symlink), as it > > > requires non-trivial changes that can be done at a later time. Like for > > > reiserfs, even if EVM is invoked, it will not provide an xattr (if it is > > > not the first to set it, its xattr will be discarded; if it is the first, > > > it does not have xattrs to calculate the HMAC on). > > > > > > Finally, modify the handling of the return value from > > > ocfs2_init_security_get(). As security_inode_init_security() does not > > > return -EOPNOTSUPP, remove this case and directly handle the error if the > > > return value is not zero. > > > > > > However, the previous case of receiving -EOPNOTSUPP should be still > > > taken into account, as security_inode_init_security() could return zero > > > without setting xattrs and ocfs2 would consider it as if the xattr was set. > > > > > > Instead, if security_inode_init_security() returned zero, look at the xattr > > > if it was set, and behave accordingly, i.e. set si->enable to zero to > > > notify to the functions following ocfs2_init_security_get() that the xattr > > > is not available (same as if security_old_inode_init_security() returned > > > -EOPNOTSUPP). > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > > > > My previous review missed a couple of concerns. > > > > > --- > > > fs/ocfs2/namei.c | 18 ++++++------------ > > > fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- > > > 2 files changed, 32 insertions(+), 16 deletions(-) > > > > > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > > > index 05f32989bad6..55fba81cd2d1 100644 > > > --- a/fs/ocfs2/namei.c > > > +++ b/fs/ocfs2/namei.c > > > @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > > int want_meta = 0; > > > int xattr_credits = 0; > > > struct ocfs2_security_xattr_info si = { > > > + .name = NULL, > > > .enable = 1, > > > }; > > > int did_quota_inode = 0; > > > @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > > /* get security xattr */ > > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > > if (status) { > > > - if (status == -EOPNOTSUPP) > > > - si.enable = 0; > > > - else { > > > - mlog_errno(status); > > > - goto leave; > > > - } > > > > Although security_inode_init_security() does not return -EOPNOTSUPP, > > ocfs2_init_security_get() could. Refer to commit 8154da3d2114 ("ocfs2: > > Add incompatible flag for extended attribute"). It was added as a > > temporary solution back in 2008, so it is highly unlikely that it is > > still needed. > > > > > + mlog_errno(status); > > > + goto leave; > > > > Without the -EOPNOTSUPP test, ocfs2_mknod() would not create the inode; > > and similarly ocfs2_symlink(), below, would not create the symlink. It > > would be safer not to remove the -EOPNOTSUPP test. > > > > > } > > > > > > /* calculate meta data/clusters for setting security and acl xattr */ > > > @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > > int want_clusters = 0; > > > int xattr_credits = 0; > > > struct ocfs2_security_xattr_info si = { > > > + .name = NULL, > > > .enable = 1, > > > }; > > > int did_quota = 0, did_quota_inode = 0; > > > @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > > /* get security xattr */ > > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > > if (status) { > > > - if (status == -EOPNOTSUPP) > > > - si.enable = 0; > > > - else { > > > - mlog_errno(status); > > > - goto bail; > > > - } > > > + mlog_errno(status); > > > + goto bail; > > > } > > > > > > /* calculate meta data/clusters for setting security xattr */ > > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > > index 95d0611c5fc7..55699c573541 100644 > > > --- a/fs/ocfs2/xattr.c > > > +++ b/fs/ocfs2/xattr.c > > > @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, > > > static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, > > > void *fs_info) > > > { > > > + struct ocfs2_security_xattr_info *si = fs_info; > > > const struct xattr *xattr; > > > int err = 0; > > > > > > + if (si) { > > > + si->value = kmemdup(xattr_array->value, xattr_array->value_len, > > > + GFP_KERNEL); > > > + if (!si->value) > > > + return -ENOMEM; > > > + > > > + si->name = xattr_array->name; > > > + si->value_len = xattr_array->value_len; > > > + return 0; > > > + } > > > + > > > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > > > err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, > > > xattr->name, xattr->value, > > > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > > > const struct qstr *qstr, > > > struct ocfs2_security_xattr_info *si) > > > { > > > + int ret; > > > + > > > /* check whether ocfs2 support feature xattr */ > > > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > > > return -EOPNOTSUPP; > > > - if (si) > > > - return security_old_inode_init_security(inode, dir, qstr, > > > - &si->name, &si->value, > > > - &si->value_len); > > > + if (si) { > > > + ret = security_inode_init_security(inode, dir, qstr, > > > + &ocfs2_initxattrs, si); > > > > The "if (unlikely(IS_PRIVATE(inode))" test exists in both > > security_old_inode_init_security() and security_inode_init_security(), > > but return different values. In the former case, it returns > > -EOPNOTSUPP. In the latter case, it returns 0. The question is > > whether or not we need to be concerned about private inodes on ocfs2. > > If private inodes on ocfs2 are possible, then ocsf2_mknod() or > > ocfs2_symlink() would fail to create the inode or symlink. > > Correction, previously when returning -EOPNOTSUPP for private inodes, > xattrs would not be wrriten. By returning 0 without setting si->enable > to 0, xattrs will be written. Ok, but if there is a private inode, we would be setting si->enable to zero. Should be ok, I guess. Thanks Roberto > > > + /* > > > + * security_inode_init_security() does not return -EOPNOTSUPP, > > > + * we have to check the xattr ourselves. > > > + */ > > > + if (!ret && !si->name) > > > + si->enable = 0; > > > + > > > + return ret; > > > + } > > > > > > return security_inode_init_security(inode, dir, qstr, > > > &ocfs2_initxattrs, NULL);
> > > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > > > index 95d0611c5fc7..55699c573541 100644 > > > > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > > > > const struct qstr *qstr, > > > > struct ocfs2_security_xattr_info *si) > > > > { > > > > + int ret; > > > > + > > > > /* check whether ocfs2 support feature xattr */ > > > > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > > > > return -EOPNOTSUPP; > > > > - if (si) > > > > - return security_old_inode_init_security(inode, dir, qstr, > > > > - &si->name, &si->value, > > > > - &si->value_len); > > > > + if (si) { > > > > + ret = security_inode_init_security(inode, dir, qstr, > > > > + &ocfs2_initxattrs, si); > > > > > > The "if (unlikely(IS_PRIVATE(inode))" test exists in both > > > security_old_inode_init_security() and security_inode_init_security(), > > > but return different values. In the former case, it returns > > > -EOPNOTSUPP. In the latter case, it returns 0. The question is > > > whether or not we need to be concerned about private inodes on ocfs2. > > > If private inodes on ocfs2 are possible, then ocsf2_mknod() or > > > ocfs2_symlink() would fail to create the inode or symlink. > > > > Correction, previously when returning -EOPNOTSUPP for private inodes, > > xattrs would not be wrriten. By returning 0 without setting si->enable > > to 0, xattrs will be written. > > Ok, but if there is a private inode, we would be setting si->enable to > zero. Should be ok, I guess. si->enable is being set to zero, below, but is conditional on !si- >name. This is the last concern, otherwise the patch set looks good. > > > > + /* > > > > + * security_inode_init_security() does not return -EOPNOTSUPP, > > > > + * we have to check the xattr ourselves. > > > > + */ > > > > + if (!ret && !si->name) > > > > + si->enable = 0; > > > > + > > > > + return ret; > > > > + } > > > > > > > > return security_inode_init_security(inode, dir, qstr, > > > > &ocfs2_initxattrs, NULL); >
On Mon, 2023-02-20 at 06:08 -0500, Mimi Zohar wrote: > > > > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > > > > index 95d0611c5fc7..55699c573541 100644 > > > > > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > > > > > const struct qstr *qstr, > > > > > struct ocfs2_security_xattr_info *si) > > > > > { > > > > > + int ret; > > > > > + > > > > > /* check whether ocfs2 support feature xattr */ > > > > > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > > > > > return -EOPNOTSUPP; > > > > > - if (si) > > > > > - return security_old_inode_init_security(inode, dir, qstr, > > > > > - &si->name, &si->value, > > > > > - &si->value_len); > > > > > + if (si) { > > > > > + ret = security_inode_init_security(inode, dir, qstr, > > > > > + &ocfs2_initxattrs, si); > > > > > > > > The "if (unlikely(IS_PRIVATE(inode))" test exists in both > > > > security_old_inode_init_security() and security_inode_init_security(), > > > > but return different values. In the former case, it returns > > > > -EOPNOTSUPP. In the latter case, it returns 0. The question is > > > > whether or not we need to be concerned about private inodes on ocfs2. > > > > If private inodes on ocfs2 are possible, then ocsf2_mknod() or > > > > ocfs2_symlink() would fail to create the inode or symlink. > > > > > > Correction, previously when returning -EOPNOTSUPP for private inodes, > > > xattrs would not be wrriten. By returning 0 without setting si->enable > > > to 0, xattrs will be written. > > > > Ok, but if there is a private inode, we would be setting si->enable to > > zero. Should be ok, I guess. > > si->enable is being set to zero, below, but is conditional on !si- > > name. > > This is the last concern, otherwise the patch set looks good. Uhm, if the inode is private, security_inode_init_security() will immediately return. So, the condition !si->name should be always true. Thanks Roberto > > > > > + /* > > > > > + * security_inode_init_security() does not return -EOPNOTSUPP, > > > > > + * we have to check the xattr ourselves. > > > > > + */ > > > > > + if (!ret && !si->name) > > > > > + si->enable = 0; > > > > > + > > > > > + return ret; > > > > > + } > > > > > > > > > > return security_inode_init_security(inode, dir, qstr, > > > > > &ocfs2_initxattrs, NULL);
On Mon, 2023-02-20 at 13:20 +0100, Roberto Sassu wrote: > On Mon, 2023-02-20 at 06:08 -0500, Mimi Zohar wrote: > > > > > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > > > > > index 95d0611c5fc7..55699c573541 100644 > > > > > > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > > > > > > const struct qstr *qstr, > > > > > > struct ocfs2_security_xattr_info *si) > > > > > > { > > > > > > + int ret; > > > > > > + > > > > > > /* check whether ocfs2 support feature xattr */ > > > > > > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > > > > > > return -EOPNOTSUPP; > > > > > > - if (si) > > > > > > - return security_old_inode_init_security(inode, dir, qstr, > > > > > > - &si->name, &si->value, > > > > > > - &si->value_len); > > > > > > + if (si) { > > > > > > + ret = security_inode_init_security(inode, dir, qstr, > > > > > > + &ocfs2_initxattrs, si); > > > > > > > > > > The "if (unlikely(IS_PRIVATE(inode))" test exists in both > > > > > security_old_inode_init_security() and security_inode_init_security(), > > > > > but return different values. In the former case, it returns > > > > > -EOPNOTSUPP. In the latter case, it returns 0. The question is > > > > > whether or not we need to be concerned about private inodes on ocfs2. > > > > > If private inodes on ocfs2 are possible, then ocsf2_mknod() or > > > > > ocfs2_symlink() would fail to create the inode or symlink. > > > > > > > > Correction, previously when returning -EOPNOTSUPP for private inodes, > > > > xattrs would not be wrriten. By returning 0 without setting si->enable > > > > to 0, xattrs will be written. > > > > > > Ok, but if there is a private inode, we would be setting si->enable to > > > zero. Should be ok, I guess. > > > > si->enable is being set to zero, below, but is conditional on !si- > > > name. > > > > This is the last concern, otherwise the patch set looks good. > > Uhm, if the inode is private, security_inode_init_security() will > immediately return. So, the condition !si->name should be always true. Ok, thanks for checking. > > > > > > > + /* > > > > > > + * security_inode_init_security() does not return -EOPNOTSUPP, > > > > > > + * we have to check the xattr ourselves. > > > > > > + */ > > > > > > + if (!ret && !si->name) > > > > > > + si->enable = 0; > > > > > > + > > > > > > + return ret; > > > > > > + } > > > > > > > > > > > > return security_inode_init_security(inode, dir, qstr, > > > > > > &ocfs2_initxattrs, NULL); >
Hi, Sorry for the late reply. I don't have much background on this thread. It seems that we have to check EOPNOTSUPP since ocfs2_init_security_get() may return EOPNOTSUPP if it doesn't support extended attribute feature for backward compatibility. Other looks good. So with above comments addressed, you can add: Acked-by: Joseph Qi <joseph.qi@linux.alibaba.com> On 1/10/23 4:55 PM, Roberto Sassu wrote: > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: >> From: Roberto Sassu <roberto.sassu@huawei.com> >> >> In preparation for removing security_old_inode_init_security(), switch to >> security_inode_init_security(). >> >> Extend the existing ocfs2_initxattrs() to take the >> ocfs2_security_xattr_info structure from fs_info, and populate the >> name/value/len triple with the first xattr provided by LSMs. > > Hi Mark, Joel, Joseph > > some time ago I sent this patch set to switch to the newer > function security_inode_init_security(). Almost all the other parts of > this patch set have been reviewed, and the patch set itself should be > ready to be merged. > > I kindly ask if you could have a look at this patch and give your > Reviewed-by, so that Paul could take the patch set. > > Thanks a lot! > > Roberto > >> As fs_info was not used before, ocfs2_initxattrs() can now handle the case >> of replicating the behavior of security_old_inode_init_security(), i.e. >> just obtaining the xattr, in addition to setting all xattrs provided by >> LSMs. >> >> Supporting multiple xattrs is not currently supported where >> security_old_inode_init_security() was called (mknod, symlink), as it >> requires non-trivial changes that can be done at a later time. Like for >> reiserfs, even if EVM is invoked, it will not provide an xattr (if it is >> not the first to set it, its xattr will be discarded; if it is the first, >> it does not have xattrs to calculate the HMAC on). >> >> Finally, modify the handling of the return value from >> ocfs2_init_security_get(). As security_inode_init_security() does not >> return -EOPNOTSUPP, remove this case and directly handle the error if the >> return value is not zero. >> >> However, the previous case of receiving -EOPNOTSUPP should be still >> taken into account, as security_inode_init_security() could return zero >> without setting xattrs and ocfs2 would consider it as if the xattr was set. >> >> Instead, if security_inode_init_security() returned zero, look at the xattr >> if it was set, and behave accordingly, i.e. set si->enable to zero to >> notify to the functions following ocfs2_init_security_get() that the xattr >> is not available (same as if security_old_inode_init_security() returned >> -EOPNOTSUPP). >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> fs/ocfs2/namei.c | 18 ++++++------------ >> fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- >> 2 files changed, 32 insertions(+), 16 deletions(-) >> >> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >> index 05f32989bad6..55fba81cd2d1 100644 >> --- a/fs/ocfs2/namei.c >> +++ b/fs/ocfs2/namei.c >> @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, >> int want_meta = 0; >> int xattr_credits = 0; >> struct ocfs2_security_xattr_info si = { >> + .name = NULL, >> .enable = 1, >> }; >> int did_quota_inode = 0; >> @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, >> /* get security xattr */ >> status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); >> if (status) { >> - if (status == -EOPNOTSUPP) >> - si.enable = 0; >> - else { >> - mlog_errno(status); >> - goto leave; >> - } >> + mlog_errno(status); >> + goto leave; >> } >> >> /* calculate meta data/clusters for setting security and acl xattr */ >> @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, >> int want_clusters = 0; >> int xattr_credits = 0; >> struct ocfs2_security_xattr_info si = { >> + .name = NULL, >> .enable = 1, >> }; >> int did_quota = 0, did_quota_inode = 0; >> @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, >> /* get security xattr */ >> status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); >> if (status) { >> - if (status == -EOPNOTSUPP) >> - si.enable = 0; >> - else { >> - mlog_errno(status); >> - goto bail; >> - } >> + mlog_errno(status); >> + goto bail; >> } >> >> /* calculate meta data/clusters for setting security xattr */ >> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c >> index 95d0611c5fc7..55699c573541 100644 >> --- a/fs/ocfs2/xattr.c >> +++ b/fs/ocfs2/xattr.c >> @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, >> static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, >> void *fs_info) >> { >> + struct ocfs2_security_xattr_info *si = fs_info; >> const struct xattr *xattr; >> int err = 0; >> >> + if (si) { >> + si->value = kmemdup(xattr_array->value, xattr_array->value_len, >> + GFP_KERNEL); >> + if (!si->value) >> + return -ENOMEM; >> + >> + si->name = xattr_array->name; >> + si->value_len = xattr_array->value_len; >> + return 0; >> + } >> + >> for (xattr = xattr_array; xattr->name != NULL; xattr++) { >> err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, >> xattr->name, xattr->value, >> @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, >> const struct qstr *qstr, >> struct ocfs2_security_xattr_info *si) >> { >> + int ret; >> + >> /* check whether ocfs2 support feature xattr */ >> if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) >> return -EOPNOTSUPP; >> - if (si) >> - return security_old_inode_init_security(inode, dir, qstr, >> - &si->name, &si->value, >> - &si->value_len); >> + if (si) { >> + ret = security_inode_init_security(inode, dir, qstr, >> + &ocfs2_initxattrs, si); >> + /* >> + * security_inode_init_security() does not return -EOPNOTSUPP, >> + * we have to check the xattr ourselves. >> + */ >> + if (!ret && !si->name) >> + si->enable = 0; >> + >> + return ret; >> + } >> >> return security_inode_init_security(inode, dir, qstr, >> &ocfs2_initxattrs, NULL);
On Tue, 2023-02-21 at 14:45 +0800, Joseph Qi wrote: > Hi, > > Sorry for the late reply. > > I don't have much background on this thread. It seems that we have to > check EOPNOTSUPP since ocfs2_init_security_get() may return EOPNOTSUPP > if it doesn't support extended attribute feature for backward > compatibility. Hi Joseph yes, I already reintroduced the check. Thanks for having a look. Roberto > Other looks good. So with above comments addressed, you can add: > Acked-by: Joseph Qi <joseph.qi@linux.alibaba.com> > > On 1/10/23 4:55 PM, Roberto Sassu wrote: > > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > In preparation for removing security_old_inode_init_security(), switch to > > > security_inode_init_security(). > > > > > > Extend the existing ocfs2_initxattrs() to take the > > > ocfs2_security_xattr_info structure from fs_info, and populate the > > > name/value/len triple with the first xattr provided by LSMs. > > > > Hi Mark, Joel, Joseph > > > > some time ago I sent this patch set to switch to the newer > > function security_inode_init_security(). Almost all the other parts of > > this patch set have been reviewed, and the patch set itself should be > > ready to be merged. > > > > I kindly ask if you could have a look at this patch and give your > > Reviewed-by, so that Paul could take the patch set. > > > > Thanks a lot! > > > > Roberto > > > > > As fs_info was not used before, ocfs2_initxattrs() can now handle the case > > > of replicating the behavior of security_old_inode_init_security(), i.e. > > > just obtaining the xattr, in addition to setting all xattrs provided by > > > LSMs. > > > > > > Supporting multiple xattrs is not currently supported where > > > security_old_inode_init_security() was called (mknod, symlink), as it > > > requires non-trivial changes that can be done at a later time. Like for > > > reiserfs, even if EVM is invoked, it will not provide an xattr (if it is > > > not the first to set it, its xattr will be discarded; if it is the first, > > > it does not have xattrs to calculate the HMAC on). > > > > > > Finally, modify the handling of the return value from > > > ocfs2_init_security_get(). As security_inode_init_security() does not > > > return -EOPNOTSUPP, remove this case and directly handle the error if the > > > return value is not zero. > > > > > > However, the previous case of receiving -EOPNOTSUPP should be still > > > taken into account, as security_inode_init_security() could return zero > > > without setting xattrs and ocfs2 would consider it as if the xattr was set. > > > > > > Instead, if security_inode_init_security() returned zero, look at the xattr > > > if it was set, and behave accordingly, i.e. set si->enable to zero to > > > notify to the functions following ocfs2_init_security_get() that the xattr > > > is not available (same as if security_old_inode_init_security() returned > > > -EOPNOTSUPP). > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > > > --- > > > fs/ocfs2/namei.c | 18 ++++++------------ > > > fs/ocfs2/xattr.c | 30 ++++++++++++++++++++++++++---- > > > 2 files changed, 32 insertions(+), 16 deletions(-) > > > > > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > > > index 05f32989bad6..55fba81cd2d1 100644 > > > --- a/fs/ocfs2/namei.c > > > +++ b/fs/ocfs2/namei.c > > > @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > > int want_meta = 0; > > > int xattr_credits = 0; > > > struct ocfs2_security_xattr_info si = { > > > + .name = NULL, > > > .enable = 1, > > > }; > > > int did_quota_inode = 0; > > > @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, > > > /* get security xattr */ > > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > > if (status) { > > > - if (status == -EOPNOTSUPP) > > > - si.enable = 0; > > > - else { > > > - mlog_errno(status); > > > - goto leave; > > > - } > > > + mlog_errno(status); > > > + goto leave; > > > } > > > > > > /* calculate meta data/clusters for setting security and acl xattr */ > > > @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > > int want_clusters = 0; > > > int xattr_credits = 0; > > > struct ocfs2_security_xattr_info si = { > > > + .name = NULL, > > > .enable = 1, > > > }; > > > int did_quota = 0, did_quota_inode = 0; > > > @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, > > > /* get security xattr */ > > > status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); > > > if (status) { > > > - if (status == -EOPNOTSUPP) > > > - si.enable = 0; > > > - else { > > > - mlog_errno(status); > > > - goto bail; > > > - } > > > + mlog_errno(status); > > > + goto bail; > > > } > > > > > > /* calculate meta data/clusters for setting security xattr */ > > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > > index 95d0611c5fc7..55699c573541 100644 > > > --- a/fs/ocfs2/xattr.c > > > +++ b/fs/ocfs2/xattr.c > > > @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, > > > static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, > > > void *fs_info) > > > { > > > + struct ocfs2_security_xattr_info *si = fs_info; > > > const struct xattr *xattr; > > > int err = 0; > > > > > > + if (si) { > > > + si->value = kmemdup(xattr_array->value, xattr_array->value_len, > > > + GFP_KERNEL); > > > + if (!si->value) > > > + return -ENOMEM; > > > + > > > + si->name = xattr_array->name; > > > + si->value_len = xattr_array->value_len; > > > + return 0; > > > + } > > > + > > > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > > > err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, > > > xattr->name, xattr->value, > > > @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, > > > const struct qstr *qstr, > > > struct ocfs2_security_xattr_info *si) > > > { > > > + int ret; > > > + > > > /* check whether ocfs2 support feature xattr */ > > > if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) > > > return -EOPNOTSUPP; > > > - if (si) > > > - return security_old_inode_init_security(inode, dir, qstr, > > > - &si->name, &si->value, > > > - &si->value_len); > > > + if (si) { > > > + ret = security_inode_init_security(inode, dir, qstr, > > > + &ocfs2_initxattrs, si); > > > + /* > > > + * security_inode_init_security() does not return -EOPNOTSUPP, > > > + * we have to check the xattr ourselves. > > > + */ > > > + if (!ret && !si->name) > > > + si->enable = 0; > > > + > > > + return ret; > > > + } > > > > > > return security_inode_init_security(inode, dir, qstr, > > > &ocfs2_initxattrs, NULL);
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c index 05f32989bad6..55fba81cd2d1 100644 --- a/fs/ocfs2/namei.c +++ b/fs/ocfs2/namei.c @@ -242,6 +242,7 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, int want_meta = 0; int xattr_credits = 0; struct ocfs2_security_xattr_info si = { + .name = NULL, .enable = 1, }; int did_quota_inode = 0; @@ -315,12 +316,8 @@ static int ocfs2_mknod(struct user_namespace *mnt_userns, /* get security xattr */ status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); if (status) { - if (status == -EOPNOTSUPP) - si.enable = 0; - else { - mlog_errno(status); - goto leave; - } + mlog_errno(status); + goto leave; } /* calculate meta data/clusters for setting security and acl xattr */ @@ -1805,6 +1802,7 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, int want_clusters = 0; int xattr_credits = 0; struct ocfs2_security_xattr_info si = { + .name = NULL, .enable = 1, }; int did_quota = 0, did_quota_inode = 0; @@ -1875,12 +1873,8 @@ static int ocfs2_symlink(struct user_namespace *mnt_userns, /* get security xattr */ status = ocfs2_init_security_get(inode, dir, &dentry->d_name, &si); if (status) { - if (status == -EOPNOTSUPP) - si.enable = 0; - else { - mlog_errno(status); - goto bail; - } + mlog_errno(status); + goto bail; } /* calculate meta data/clusters for setting security xattr */ diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c index 95d0611c5fc7..55699c573541 100644 --- a/fs/ocfs2/xattr.c +++ b/fs/ocfs2/xattr.c @@ -7259,9 +7259,21 @@ static int ocfs2_xattr_security_set(const struct xattr_handler *handler, static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array, void *fs_info) { + struct ocfs2_security_xattr_info *si = fs_info; const struct xattr *xattr; int err = 0; + if (si) { + si->value = kmemdup(xattr_array->value, xattr_array->value_len, + GFP_KERNEL); + if (!si->value) + return -ENOMEM; + + si->name = xattr_array->name; + si->value_len = xattr_array->value_len; + return 0; + } + for (xattr = xattr_array; xattr->name != NULL; xattr++) { err = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY, xattr->name, xattr->value, @@ -7277,13 +7289,23 @@ int ocfs2_init_security_get(struct inode *inode, const struct qstr *qstr, struct ocfs2_security_xattr_info *si) { + int ret; + /* check whether ocfs2 support feature xattr */ if (!ocfs2_supports_xattr(OCFS2_SB(dir->i_sb))) return -EOPNOTSUPP; - if (si) - return security_old_inode_init_security(inode, dir, qstr, - &si->name, &si->value, - &si->value_len); + if (si) { + ret = security_inode_init_security(inode, dir, qstr, + &ocfs2_initxattrs, si); + /* + * security_inode_init_security() does not return -EOPNOTSUPP, + * we have to check the xattr ourselves. + */ + if (!ret && !si->name) + si->enable = 0; + + return ret; + } return security_inode_init_security(inode, dir, qstr, &ocfs2_initxattrs, NULL);