On Mon, 2023-02-20 at 06:08 -0500, Mimi Zohar wrote:

quoted

quoted

quoted

quoted

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-

quoted

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

quoted

quoted

quoted

quoted

+		/*
+		 * 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);