Patchwork [06/11,RESEND] e4defrag: Allow user who has read+write access to defrag

login
register
mail settings
Submitter Kazuya Mio
Date June 15, 2011, 6:36 a.m.
Message ID <4DF852F8.2020208@sx.jp.nec.com>
Download mbox | patch
Permalink /patch/100484/
State Superseded
Headers show

Comments

Kazuya Mio - June 15, 2011, 6:36 a.m.
Anyone who has read+write access can defrag the file for this fix.
Currently, non-root user needs owner authority to defrag the file. But non-root
user who is not owner might have read+write access.

Signed-off-by: Kazuya Mio <k-mio@sx.jp.nec.com>
---
 misc/e4defrag.8.in |    3 +--
 misc/e4defrag.c    |   25 ++++---------------------
 2 files changed, 5 insertions(+), 23 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger - June 17, 2011, 9:10 a.m.
On 2011-06-15, at 12:36 AM, Kazuya Mio wrote:
> Anyone who has read+write access can defrag the file for this fix.
> Currently, non-root user needs owner authority to defrag the file. But non-root
> user who is not owner might have read+write access.

I was looking at this code, and doing any kind of permission checking in
userspace makes no sense.  Anyone could download the code and recompile
it without this check, so it is clear that all permission checking has
to happen in the kernel.

I agree that anyone with read+write access to the file can corrupt it,
just as badly as if they wrote garbage into the file, so it seems this
should be enough permission to also run defragmentation on the file.
It is good that you have removed these checks.

> @@ -466,6 +465,7 @@ static int check_free_size(int fd, const char *file,
> 	ext4_fsblk_t	free_blk_count;
> 	struct statfs64	fsbuf;
> +	uid_t		current_uid = getuid();
> 
> 	if (fstatfs64(fd, &fsbuf) < 0) {
> 		if (mode_flag & DETAIL) {

This one last usage is also incorrect.  It assumes that ROOT_UID is the
only one that can access the "reserved" space in the filesystem.  In fact,
it is possible to set s_def_resuid and s_def_resgid in the superblock to
allow anyone with that UID or GID to access the reserved space.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/misc/e4defrag.8.in b/misc/e4defrag.8.in
index 81adc29..1159fd6 100644
--- a/misc/e4defrag.8.in
+++ b/misc/e4defrag.8.in
@@ -43,9 +43,8 @@  is a device or a mount point,
 .B e4defrag
 doesn't defragment files in mount point of other device.
 .PP
-Non-privileged users can execute
 .B e4defrag
-to their own file.
+can be called for the file only if read and write access are allowed.
 .SH AUTHOR
 Written by Akira Fujita <a-fujita@rs.jp.nec.com> and Takashi Sato
 <t-sato@yk.jp.nec.com>.
diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index 417639d..f4ced9a 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -148,7 +148,6 @@  int	block_size;
 int	extents_before_defrag;
 int	extents_after_defrag;
 int	mode_flag;
-unsigned int	current_uid;
 unsigned int	defraged_file_count;
 unsigned int	frag_files_before_defrag;
 unsigned int	frag_files_after_defrag;
@@ -466,6 +465,7 @@  static int check_free_size(int fd, const char *file, ext4_fsblk_t blk_count)
 {
 	ext4_fsblk_t	free_blk_count;
 	struct statfs64	fsbuf;
+	uid_t		current_uid = getuid();
 
 	if (fstatfs64(fd, &fsbuf) < 0) {
 		if (mode_flag & DETAIL) {
@@ -517,13 +517,12 @@  static int file_frag_count(int fd)
  * file_check() -	Check file's attributes.
  *
  * @fd:			defrag target file's descriptor.
- * @buf:		a pointer of the struct stat64.
  * @file:		file name.
  * @extents:		file extents.
  * @blk_count:		file blocks.
  */
-static int file_check(int fd, const struct stat64 *buf, const char *file,
-		int extents, ext4_fsblk_t blk_count)
+static int file_check(int fd, const char *file, int extents,
+					ext4_fsblk_t blk_count)
 {
 	int	ret;
 	struct flock	lock;
@@ -547,20 +546,6 @@  static int file_check(int fd, const struct stat64 *buf, const char *file,
 		return -1;
 	}
 
-	/* Access authority */
-	if (current_uid != ROOT_UID &&
-		buf->st_uid != current_uid) {
-		if (mode_flag & DETAIL) {
-			printf("\033[79;0H\033[K[%u/%u] \"%s\"\t\t"
-				"  extents: %d -> %d\n", defraged_file_count,
-				total_count, file, extents, extents);
-			IN_FTW_PRINT_ERR_MSG(
-				"File is not current user's file"
-				" or current user is not root");
-		}
-		return -1;
-	}
-
 	/* Lock status */
 	if (fcntl(fd, F_GETLK, &lock) < 0) {
 		if (mode_flag & DETAIL) {
@@ -1040,7 +1025,7 @@  static int file_defrag(const char *file, const struct stat64 *buf,
 	file_frags_start = get_exts_count(orig_list);
 
 	blk_count = get_file_blocks(orig_list);
-	if (file_check(fd, buf, file, file_frags_start, blk_count) < 0)
+	if (file_check(fd, file, file_frags_start, blk_count) < 0)
 		goto out;
 
 	if (fsync(fd) < 0) {
@@ -1243,8 +1228,6 @@  int main(int argc, char *argv[])
 	if (argc == optind)
 		goto out;
 
-	current_uid = getuid();
-
 	/* Main process */
 	for (i = optind; i < argc; i++) {
 		succeed_cnt = 0;