[Ocfs2-devel] [PATCH] ocfs2: fix ocfs2_init_security_and_acl() to initialize acl correctly

[Ocfs2-devel] [PATCH] ocfs2: fix ocfs2_init_security_and_acl() to initialize acl correctly

[Jeff Liu]

We need to re-initialize the security if it isn't preserved for ocfs2_reflink().
however, the code logic is broken at ocfs2_init_security_and_acl() although
ocfs2_init_security_get() succeed, as a result, ocfs2_acl_init() does not involked.

Note this was introduced by commit:
commit 9d8f13ba3f4833219e50767b022b82cd0da930eb
  security: new security_inode_init_security API adds function callback

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Reviewed-by: Tao Ma <boyu.mt@taobao.com>

---
 fs/ocfs2/xattr.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 0ba9ea1..2e3ea30 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -7189,7 +7189,7 @@ int ocfs2_init_security_and_acl(struct inode *dir,
 	struct buffer_head *dir_bh = NULL;
 
 	ret = ocfs2_init_security_get(inode, dir, qstr, NULL);
-	if (!ret) {
+	if (ret) {
 		mlog_errno(ret);
 		goto leave;
 	}
-- 
1.7.9.5

[Ocfs2-devel] [PATCH] ocfs2: fix ocfs2_init_security_and_acl() to initialize acl correctly

[Andrew Morton]

On Fri, 22 Feb 2013 19:15:46 +0800
Jeff Liu <jeff.liu@oracle.com> wrote:

> We need to re-initialize the security if it isn't preserved for ocfs2_reflink().
> however, the code logic is broken at ocfs2_init_security_and_acl() although
> ocfs2_init_security_get() succeed, as a result, ocfs2_acl_init() does not involked.

When writing a changelog, please describe the end-user-visible effects
of the bug, so that others can more easily decide which kernel
version(s) should be fixed, and so that downstream kernel maintainers
can more easily work out whether this patch will fix a problem which
they or their customers are observing.

[Ocfs2-devel] [PATCH] ocfs2: fix ocfs2_init_security_and_acl() to initialize acl correctly

[Jeff Liu]

Hi Andrew,

On 02/23/2013 05:21 AM, Andrew Morton wrote:
> On Fri, 22 Feb 2013 19:15:46 +0800
> Jeff Liu <jeff.liu@oracle.com> wrote:
> 
>> We need to re-initialize the security if it isn't preserved for ocfs2_reflink().
>> however, the code logic is broken at ocfs2_init_security_and_acl() although
>> ocfs2_init_security_get() succeed, as a result, ocfs2_acl_init() does not involked.
> 
> When writing a changelog, please describe the end-user-visible effects
> of the bug, so that others can more easily decide which kernel
> version(s) should be fixed, and so that downstream kernel maintainers
> can more easily work out whether this patch will fix a problem which
> they or their customers are observing.
Thanks for your teaching, I'll take care of it next time.

Regards,
-Jeff

[Ocfs2-devel] [PATCH] ocfs2: fix ocfs2_init_security_and_acl() to initialize acl correctly

[Andrew Morton]

On Sat, 23 Feb 2013 10:12:10 +0800 Jeff Liu <jeff.liu@oracle.com> wrote:

> Hi Andrew,
> 
> On 02/23/2013 05:21 AM, Andrew Morton wrote:
> > On Fri, 22 Feb 2013 19:15:46 +0800
> > Jeff Liu <jeff.liu@oracle.com> wrote:
> > 
> >> We need to re-initialize the security if it isn't preserved for ocfs2_reflink().
> >> however, the code logic is broken at ocfs2_init_security_and_acl() although
> >> ocfs2_init_security_get() succeed, as a result, ocfs2_acl_init() does not involked.
> > 
> > When writing a changelog, please describe the end-user-visible effects
> > of the bug, so that others can more easily decide which kernel
> > version(s) should be fixed, and so that downstream kernel maintainers
> > can more easily work out whether this patch will fix a problem which
> > they or their customers are observing.
> Thanks for your teaching, I'll take care of it next time.
> 

Well OK, but please provide this info for this particular patch!  I
still don't know if we should backport it into -stable kernels.

[Ocfs2-devel] [PATCH] ocfs2: fix ocfs2_init_security_and_acl() to initialize acl correctly

[Jeff Liu]

On 02/23/2013 10:31 AM, Andrew Morton wrote:
> On Sat, 23 Feb 2013 10:12:10 +0800 Jeff Liu <jeff.liu@oracle.com> wrote:
> 
>> Hi Andrew,
>>
>> On 02/23/2013 05:21 AM, Andrew Morton wrote:
>>> On Fri, 22 Feb 2013 19:15:46 +0800
>>> Jeff Liu <jeff.liu@oracle.com> wrote:
>>>
>>>> We need to re-initialize the security if it isn't preserved for ocfs2_reflink().
>>>> however, the code logic is broken at ocfs2_init_security_and_acl() although
>>>> ocfs2_init_security_get() succeed, as a result, ocfs2_acl_init() does not involked.
>>>
>>> When writing a changelog, please describe the end-user-visible effects
>>> of the bug, so that others can more easily decide which kernel
>>> version(s) should be fixed, and so that downstream kernel maintainers
>>> can more easily work out whether this patch will fix a problem which
>>> they or their customers are observing.
>> Thanks for your teaching, I'll take care of it next time.
>>
> 
> Well OK, but please provide this info for this particular patch!  I
> still don't know if we should backport it into -stable kernels.

Could you please check the following revised changelog?

We need to re-initialize the security for a new reflinked inode with it's parent
dirs if it isn't specified to be preserved for ocfs2_reflink().  However, the code
logic is broken at ocfs2_init_security_and_acl() although ocfs2_init_security_get()
succeed.  As a result, ocfs2_acl_init() does not involked and therefore the default
ACL of parent dir was missing on the new inode.

Note this was introduced by 9d8f13ba3 ("security: new security_inode_init_security API adds function callback")

To reproduce:

set default ACL for the parent dir(ocfs2 in this case):
$ setfacl -m default:user:jeff:rwx ../ocfs2/
$ getfacl ../ocfs2/
# file: ../ocfs2/
# owner: jeff
# group: jeff
user::rwx
group::r-x
other::r-x
default:user::rwx
default:user:jeff:rwx
default:group::r-x
default:mask::rwx
default:other::r-x

$ touch a
$ getfacl a
# file: a
# owner: jeff
# group: jeff
user::rw-
group::rw-
other::r--

Before patching, create reflink file b from a, the user
default ACL entry(user:jeff:rwx)was missing:
$ ./ocfs2_reflink a b
$ getfacl b
# file: b
# owner: jeff
# group: jeff
user::rw-
group::rw-
other::r--

In this case, the end user can also observed an error message at syslog:
(ocfs2_reflink,3229,2):ocfs2_init_security_and_acl:7193 ERROR: status = 0

After applying this patch, create reflink file c from a:
$ ./ocfs2_reflink a c
$ getfacl c
# file: c
# owner: jeff
# group: jeff
user::rw-
user:jeff:rwx			#effective:rw-
group::r-x			#effective:r--
mask::rw-
other::r--

Test program:
/* Usage: reflink <source> <dest> */
#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include <string.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>

static int
reflink_file(char const *src_name, char const *dst_name,
	     bool preserve_attrs)
{
	int fd;

#ifndef REFLINK_ATTR_NONE
#  define REFLINK_ATTR_NONE 0
#endif
#ifndef REFLINK_ATTR_PRESERVE
#  define REFLINK_ATTR_PRESERVE 1
#endif
#ifndef OCFS2_IOC_REFLINK
	struct reflink_arguments {
		uint64_t old_path;
		uint64_t new_path;
		uint64_t preserve;
	};

#  define OCFS2_IOC_REFLINK _IOW ('o', 4, struct reflink_arguments)
#endif
	struct reflink_arguments args = {
		.old_path = (unsigned long) src_name,
		.new_path = (unsigned long) dst_name,
		.preserve = preserve_attrs ? REFLINK_ATTR_PRESERVE :
					     REFLINK_ATTR_NONE,
	};

	fd = open(src_name, O_RDONLY);
	if (fd < 0) {
		fprintf(stderr, "Failed to open %s: %s\n",
			src_name, strerror(errno));
		return -1;
	}

	if (ioctl(fd, OCFS2_IOC_REFLINK, &args) < 0) {
		fprintf(stderr, "Failed to reflink %s to %s: %s\n",
			src_name, dst_name, strerror(errno));
		return -1;
	}
}

int
main(int argc, char *argv[])
{
	if (argc != 3) {
		fprintf(stdout, "Usage: %s source dest\n", argv[0]);
		return 1;
	}

	return reflink_file(argv[1], argv[2], 0);
}


 

Thanks for your time!

-Jeff