Re: Minor Bug in 2.4.29 patchset and legacy SMP bug in 2.2.xx :)

Hello Peter,

your bug report and analysis nicely explains the problems reported by
Robert and Thomas. Thanks a lot.

On Fri, 2005-02-18 at 08:51, Peter K. wrote:
> Hi! I'm working with your testing brunch of acl patch for 2.4.29
> kernel and there is some bug... I'm speaking english bad, but i'll
> trying to explain.
> 
> Looking the case, when there is the xattrs on the file, but no "acl"
> attribute is set. So...first, at ext?_read_inode 
> 
> #ifdef CONFIG_EXT3_FS_POSIX_ACL 
>         if (inode->u.ext3_i.i_file_acl) {
>                 /* The filesystem is mounted with ACL support, and there 
>                    are extended attributes for this inode. However we do 
>                    not yet know whether there are actually any ACLs. */
>                 inode->u.ext3_i.i_acl = EXT3_ACL_NOT_CACHED;
>                 inode->u.ext3_i.i_default_acl = EXT3_ACL_NOT_CACHED;
>         }
> #endif 
> 
> So, i_acl == i_default_acl == EXT3_ACL_NOT_CACHED couse i_file_acl is
> used for xattr layer and posix_acl is just it's subset.
> 
> In ext3_permission ...
> 
>                 if (ei->i_acl == EXT3_ACL_NOT_CACHED) {
>                         struct posix_acl *acl =
>                                 ext3_get_acl(inode, ACL_TYPE_ACCESS);
> 
>                         if (IS_ERR(acl))
>                                 return PTR_ERR(acl);
>                         posix_acl_release(acl);
>                         if (ei->i_acl == EXT3_ACL_NOT_CACHED)
>                                 return -EIO;
>                 }
> 
> So, i_acl == ... CACHED, we will try to ext3_get_acl. and logically
> got -ENODATA inside ext3_get_acl, but in fact, the function returns 0
> outside in that case, so we got in the 3-d "if" and got  -EIO (because
> i_acl is still NOT_CACHED).
> 
> So, when we wants to get acces to the file, that have some xattr, but
> no acl xattr, we got -EIO error.
> 
> It's seems like the problem is in 
> 
>         if (retval <= 0) {
>                 if (retval == -ENODATA || retval == -ENOSYS)
> //                { /* my dirty fix */
> //                        ei->i_acl = 0;
> //                        ei->i_default_acl = 0;
>                         retval = 0; /* original code */
> //                }
>                 return ERR_PTR(retval);
> 
> 
>  the past version of patch (2.4.25) was:
> 
>       if (retval > 0)
>                 acl = ext3_acl_from_disk(value, retval);
>         else if (retval == -ENODATA || retval == -ENOSYS)
>                 acl = NULL;
> 
> And we got dup acl from posix modes here, and it works.

Yes, that's much better. How do you like the attached patch? I have also
added it to the 2.4.29 testing snapshot.

> By the way. i have used your legacy patches for 2.2.xx kernels (about
> 2 years ago) and found there SMP sync. bug, thats leads sometimes to
> deadlocking (there was race cond. ) In fact, i have not my patch right
> here (on my machine where i am typing now) but if it's interesting
> (just for fun) i can remember it =).

Well, 2.2 is totally dead as far as those patches are concerned ;)

Cheers,
-- 
Andreas Gruenbacher <[email protected]>
SUSE Labs, SUSE LINUX GMBH

_______________________________________________
acl-devel mailing list
[email protected]
http://acl.bestbits.at/mailman/listinfo/acl-devel

Index: linux-2.4.29/fs/ext2/acl.c
===================================================================
--- linux-2.4.29.orig/fs/ext2/acl.c
+++ linux-2.4.29/fs/ext2/acl.c
@@ -134,7 +134,7 @@ ext2_get_acl(struct inode *inode, int ty
 	struct ext2_inode_info *ei = EXT2_I(inode);
 	int name_index;
 	char *value;
-	struct posix_acl *acl;
+	struct posix_acl *acl = NULL;
 	int retval;
 
 	if (!IS_POSIXACL(inode))
@@ -167,18 +167,20 @@ ext2_get_acl(struct inode *inode, int ty
 		kfree(value);
 	}
 	if (retval <= 0) {
+		acl = ERR_PTR(retval);
 		if (retval == -ENODATA || retval == -ENOSYS)
-			retval = 0;
-		return ERR_PTR(retval);
+			acl = NULL;
 	}
-	switch(type) {
-		case ACL_TYPE_ACCESS:
-			ei->i_acl = posix_acl_dup(acl);
-			break;
+	if (!IS_ERR(acl)) {
+		switch(type) {
+			case ACL_TYPE_ACCESS:
+				ei->i_acl = posix_acl_dup(acl);
+				break;
 
-		case ACL_TYPE_DEFAULT:
-			ei->i_default_acl = posix_acl_dup(acl);
-			break;
+			case ACL_TYPE_DEFAULT:
+				ei->i_default_acl = posix_acl_dup(acl);
+				break;
+		}
 	}
 	return acl;
 }
Index: linux-2.4.29/fs/ext3/acl.c
===================================================================
--- linux-2.4.29.orig/fs/ext3/acl.c
+++ linux-2.4.29/fs/ext3/acl.c
@@ -137,7 +137,7 @@ ext3_get_acl(struct inode *inode, int ty
 	struct ext3_inode_info *ei = EXT3_I(inode);
 	int name_index;
 	char *value;
-	struct posix_acl *acl;
+	struct posix_acl *acl = NULL;
 	int retval;
 
 	if (!IS_POSIXACL(inode))
@@ -170,18 +170,20 @@ ext3_get_acl(struct inode *inode, int ty
 		kfree(value);
 	}
 	if (retval <= 0) {
+		acl = ERR_PTR(retval);
 		if (retval == -ENODATA || retval == -ENOSYS)
-			retval = 0;
-		return ERR_PTR(retval);
+			acl = NULL;
 	}
-	switch(type) {
-		case ACL_TYPE_ACCESS:
-			ei->i_acl = posix_acl_dup(acl);
-			break;
+	if (!IS_ERR(acl)) {
+		switch(type) {
+			case ACL_TYPE_ACCESS:
+				ei->i_acl = posix_acl_dup(acl);
+				break;
 
-		case ACL_TYPE_DEFAULT:
-			ei->i_default_acl = posix_acl_dup(acl);
-			break;
+			case ACL_TYPE_DEFAULT:
+				ei->i_default_acl = posix_acl_dup(acl);
+				break;
+		}
 	}
 	return acl;
 }