Skip to contentSkip to conversation

f2fs: relax permission for atomic/volatile ioctls

PATCH In review Jaegeuk Kim 4 participants 8 messages Jul 6, 2017 – Jul 7, 2017

Subsystems (2)
VFS F2FS file system
lore
Share & tools
lore

Jaegeuk Kim

author maintainer
#0 lore raw

PATCH 1/1 f2fs: relax permission for atomic/volatile ioctls

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c | 15 ---------------
 1 file changed, 15 deletions(-)
fs/f2fs/file.c +0 -15
index f5d6357e8360..dd8f5d2caa48 100644
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1636,9 +1633,6 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1672,9 +1666,6 @@ static int f2fs_ioc_start_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1707,9 +1698,6 @@ static int f2fs_ioc_release_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1736,9 +1724,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
maintainer
#1 lore raw
Quoted 1 line · Jaegeuk Kim

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Out of curiosity, we will lose some capable when passing through sdcardfs?

Thanks,

Quoted 60 lines
Signed-off-by Jaegeuk Kim <[email protected]>> ---
 fs/f2fs/file.c | 15 ---------------
 1 file changed, 15 deletions(-)
fs/f2fs/file.c +0 -15
index f5d6357e8360..dd8f5d2caa48 100644
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1636,9 +1633,6 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1672,9 +1666,6 @@ static int f2fs_ioc_start_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1707,9 +1698,6 @@ static int f2fs_ioc_release_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1736,9 +1724,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;
author maintainer
#2 lore raw
Quoted 4 lines · Chao Yu
Quoted 1 line · Jaegeuk Kim

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Out of curiosity, we will lose some capable when passing through sdcardfs?

I don't think so. But, it seems a test applicaion tries to access database from difference uid.

Thanks,

Quoted 63 lines

Thanks,

Quoted 60 lines
Signed-off-by Jaegeuk Kim <[email protected]>> ---
 fs/f2fs/file.c | 15 ---------------
 1 file changed, 15 deletions(-)
fs/f2fs/file.c +0 -15
index f5d6357e8360..dd8f5d2caa48 100644
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1636,9 +1633,6 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1672,9 +1666,6 @@ static int f2fs_ioc_start_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1707,9 +1698,6 @@ static int f2fs_ioc_release_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1736,9 +1724,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;
maintainer
#3 lore raw
Quoted 8 lines · Jaegeuk Kim
Quoted 4 lines · Chao Yu
Quoted 1 line · Jaegeuk Kim

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Out of curiosity, we will lose some capable when passing through sdcardfs?

I don't think so. But, it seems a test applicaion tries to access database from difference uid.

Oh, is that really allowed? if the sqlite database is public in sdcard directory, application needs to apply WRITE_EXTERNAL_STORAGE in order to add itself to sdcard_rw group, then it can access the database. Right?

Thanks,

Quoted 66 lines

Thanks,

Quoted 63 lines

Thanks,

Quoted 60 lines
Signed-off-by Jaegeuk Kim <[email protected]>> ---
 fs/f2fs/file.c | 15 ---------------
 1 file changed, 15 deletions(-)
fs/f2fs/file.c +0 -15
index f5d6357e8360..dd8f5d2caa48 100644
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1636,9 +1633,6 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1672,9 +1666,6 @@ static int f2fs_ioc_start_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1707,9 +1698,6 @@ static int f2fs_ioc_release_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1736,9 +1724,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;
#4 lore raw

Hi,
I think Sdcardfs should override task_struct cred fsuid/fsgid before calling the underlay fs operation. And it seems sdcardfs implementations misses override cred fsuid/fsgid before the ioctl operation.

Thanks.

-----邮件原件-----
发件人: Chao Yu [mailto:[email protected]]
发送时间: 2017年7月7日 8:58
收件人: Jaegeuk Kim
抄送: [email protected]; [email protected]; [email protected] 主题: Re: [f2fs-dev] [PATCH] f2fs: relax permission for atomic/volatile ioctls

Quoted 8 lines · Jaegeuk Kim
Quoted 4 lines · Chao Yu
Quoted 1 line · Jaegeuk Kim

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Out of curiosity, we will lose some capable when passing through sdcardfs?

I don't think so. But, it seems a test applicaion tries to access database from difference uid.

Oh, is that really allowed? if the sqlite database is public in sdcard directory, application needs to apply WRITE_EXTERNAL_STORAGE in order to add itself to sdcard_rw group, then it can access the database. Right?

Thanks,

Quoted 59 lines

Thanks,

Quoted 56 lines

Thanks,

Quoted 53 lines
Signed-off-by Jaegeuk Kim <[email protected]>> ---
 fs/f2fs/file.c | 15 ---------------
 1 file changed, 15 deletions(-)
fs/f2fs/file.c +0 -12
index f5d6357e8360..dd8f5d2caa48 100644
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1636,9 +1633,6 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1672,9 +1666,6 @@ static int f2fs_ioc_start_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1707,9 +1698,6 @@ static int f2fs_ioc_release_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1736,9 +1724,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
Quoted 7 lines
Quoted 7 lines

fs/f2fs/file.c static int f2fs_ioc_commit_atomic_write(struct file *filp) · line 1638

Quoted 7 lines
fs/f2fs/file.c +0 -3
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

author maintainer
#5 lore raw
Quoted 3 lines · gaoxiang (P)

Hi,
I think Sdcardfs should override task_struct cred fsuid/fsgid before calling the underlay fs operation. And it seems sdcardfs implementations misses override cred fsuid/fsgid before the ioctl operation.

Oh, good catch!

Thank you. ;)

Quoted 100 lines

Thanks.

-----邮件原件-----
发件人: Chao Yu [mailto:[email protected]]
发送时间: 2017年7月7日 8:58
收件人: Jaegeuk Kim
抄送: [email protected]; [email protected]; [email protected] 主题: Re: [f2fs-dev] [PATCH] f2fs: relax permission for atomic/volatile ioctls

Quoted 8 lines · Jaegeuk Kim
Quoted 4 lines · Chao Yu
Quoted 1 line · Jaegeuk Kim

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Out of curiosity, we will lose some capable when passing through sdcardfs?

I don't think so. But, it seems a test applicaion tries to access database from difference uid.

Oh, is that really allowed? if the sqlite database is public in sdcard directory, application needs to apply WRITE_EXTERNAL_STORAGE in order to add itself to sdcard_rw group, then it can access the database. Right?

Thanks,

Quoted 59 lines

Thanks,

Quoted 56 lines

Thanks,

Quoted 53 lines
Signed-off-by Jaegeuk Kim <[email protected]>> ---
 fs/f2fs/file.c | 15 ---------------
 1 file changed, 15 deletions(-)
fs/f2fs/file.c +0 -12
index f5d6357e8360..dd8f5d2caa48 100644
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1636,9 +1633,6 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1672,9 +1666,6 @@ static int f2fs_ioc_start_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1707,9 +1698,6 @@ static int f2fs_ioc_release_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1736,9 +1724,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
Quoted 7 lines
Quoted 7 lines

fs/f2fs/file.c static int f2fs_ioc_commit_atomic_write(struct file *filp) · line 1638

Quoted 7 lines
fs/f2fs/file.c +0 -3
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

#6 lore raw

-----邮件原件-----
发件人: Jaegeuk Kim [mailto:[email protected]]
发送时间: 2017年7月7日 10:12
收件人: gaoxiang (P)
抄送: Chao Yu; [email protected]; [email protected]; [email protected] 主题: Re: 答复: [f2fs-dev] [PATCH] f2fs: relax permission for atomic/volatile ioctls

Quoted 3 lines · gaoxiang (P)

Hi,
I think Sdcardfs should override task_struct cred fsuid/fsgid before calling the underlay fs operation. And it seems sdcardfs implementations misses override cred fsuid/fsgid before the ioctl operation.

Oh, good catch!

Thank you. ;)

;) We are fixing it internally. And the AOSP sdcardfs patch would be
https://android-review.googlesource.com/#/c/431259/
It is none of f2fs business. :) :)
Thanks.

Quoted 102 lines

Thanks.

-----邮件原件-----
发件人: Chao Yu [mailto:[email protected]]
发送时间: 2017年7月7日 8:58
收件人: Jaegeuk Kim
抄送: [email protected]; [email protected]; [email protected]
主题: Re: [f2fs-dev] [PATCH] f2fs: relax permission for atomic/volatile ioctls

Quoted 8 lines · Jaegeuk Kim
Quoted 4 lines · Chao Yu
Quoted 1 line · Jaegeuk Kim

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Out of curiosity, we will lose some capable when passing through sdcardfs?

I don't think so. But, it seems a test applicaion tries to access database from difference uid.

Oh, is that really allowed? if the sqlite database is public in sdcard directory, application needs to apply WRITE_EXTERNAL_STORAGE in order to add itself to sdcard_rw group, then it can access the database. Right?

Thanks,

Quoted 59 lines

Thanks,

Quoted 56 lines

Thanks,

Quoted 53 lines
Signed-off-by Jaegeuk Kim <[email protected]>> ---

fs/f2fs/file.c | 15 ---------------

 1 file changed, 15 deletions(-)
fs/f2fs/file.c index renamed from fs/f2fs/file.c +0 -0

f5d6357e8360..dd8f5d2caa48 100644

fs/f2fs/file.c +0 -12
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1636,9 +1633,6 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1672,9 +1666,6 @@ static int f2fs_ioc_start_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 

@@ -1707,9 +1698,6 @@ static int f2fs_ioc_release_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

@@ -1736,9 +1724,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
Quoted 7 lines
Quoted 7 lines

fs/f2fs/file.c static int f2fs_ioc_commit_atomic_write(struct file *filp) · line 1638

Quoted 7 lines
fs/f2fs/file.c +0 -3
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;

----------------------------------------------------------------------
-------- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

#7 lore raw
In reply to Jaegeuk Kim
Quoted 12 lines · Jaegeuk Kim

This patch allows atomic/volatile ioctls for sqlite under sdcardfs.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c | 15 ---------------
 1 file changed, 15 deletions(-)
fs/f2fs/file.c +0 -0
index f5d6357e8360..dd8f5d2caa48 100644

fs/f2fs/file.c static int f2fs_ioc_start_atomic_write(struct file *filp) · line 1592

Quoted 6 lines
fs/f2fs/file.c +0 -3
@@ -1587,9 +1587,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct inode *inode = file_inode(filp);
 	int ret;
 
-	if (!inode_owner_or_capable(inode))
-		return -EACCES;
-

Why not require that the file be open for writing (FMODE_WRITE)?