Skip to contentSkip to conversation

Ocfs2: do not allow fallocate on dir file

PATCH New Li Dongyang 6 participants 10 messages Mar 4, 2011 – Mar 10, 2011 OCFS2

lore
Share & tools
lore

Li Dongyang

author
#0 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

allowing fallocate() on dir file doesn't make sense, we check if we are dealing with a regular file and return -EINVAL when it's not, Thanks

Signed-off-by Li Dongyang <[email protected]>
---
 fs/ocfs2/file.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/ocfs2/file.c +5 -0
index a665195..0c68a61 100644
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 		goto out_inode_unlock;
 	}
 
+	if (!S_ISREG(inode->i_mode)) {
+		ret = -EINVAL;
+		goto out_inode_unlock;
+	}
+
 	switch (sr->l_whence) {
 	case 0: /*SEEK_SET*/
 		break;
Signature · 1 line
1.7.1
#1 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Hi Dongyang,

Looks like we've already had a check for that in
ocfs2_change_file_space().

Quoted 25 lines · Li Dongyang

allowing fallocate() on dir file doesn't make sense, we check if we are dealing with a regular file and return -EINVAL when it's not, Thanks

Signed-off-by Li Dongyang <[email protected]>
---
 fs/ocfs2/file.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/ocfs2/file.c +5 -0
index a665195..0c68a61 100644
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 		goto out_inode_unlock;
 	}
 
+	if (!S_ISREG(inode->i_mode)) {
+		ret = -EINVAL;
+		goto out_inode_unlock;
+	}
+
 	switch (sr->l_whence) {
 	case 0: /*SEEK_SET*/
 		break;
#2 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

In reply to Li Dongyang
Quoted 14 lines · Li Dongyang

allowing fallocate() on dir file doesn't make sense, we check if we are dealing with a regular file and return -EINVAL when it's not, Thanks

Signed-off-by Li Dongyang <[email protected]>
---
 fs/ocfs2/file.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
fs/ocfs2/file.c +0 -0
index a665195..0c68a61 100644

fs/ocfs2/file.c static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 7 lines
fs/ocfs2/file.c +4 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 		goto out_inode_unlock;
 	}
 
+	if (!S_ISREG(inode->i_mode)) {
+		ret = -EINVAL;
+		goto out_inode_unlock;
+	}

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

	--Mark

--
Mark Fasheh

#3 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Quoted 5 lines · Mark Fasheh
Quoted 4 lines · Li Dongyang
fs/ocfs2/file.c +0 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
  		goto out_inode_unlock;
  	}
Quoted 7 lines

fs/ocfs2/file.c static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 4 lines
fs/ocfs2/file.c +4 -0
+	if (!S_ISREG(inode->i_mode)) {
+		ret = -EINVAL;
+		goto out_inode_unlock;
+	}

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

Tristan pointed out that this check was already in ocfs2_change_file_space(). So this patch should not be required. Am I missing something?

#4 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Quoted 14 lines · Sunil Mushran
Quoted 5 lines · Mark Fasheh
Quoted 4 lines · Li Dongyang
fs/ocfs2/file.c +0 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
  		goto out_inode_unlock;
  	}
Quoted 7 lines

fs/ocfs2/file.c static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 4 lines
fs/ocfs2/file.c +4 -0
+	if (!S_ISREG(inode->i_mode)) {
+		ret = -EINVAL;
+		goto out_inode_unlock;
+	}

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

Quoted 2 lines

Tristan pointed out that this check was already in ocfs2_change_file_space(). So this patch should not be required. Am I missing something?

This patch is needed since the check is in ocfs2_change_file_space. But ocfs2_fallocate calls __ocfs2_change_file_space directly.

So maybe we should add the check here and remove the check in ocfs2_change_file_space?

Regards,
Tao

#5 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Quoted 15 lines · Tao Ma
Quoted 14 lines · Sunil Mushran
Quoted 12 lines · Mark Fasheh

fs/ocfs2/file.c static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 8 lines · Li Dongyang
fs/ocfs2/file.c +4 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
  		goto out_inode_unlock;
  	}
 
+	if (!S_ISREG(inode->i_mode)) {
+		ret = -EINVAL;
+		goto out_inode_unlock;
+	}

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

Quoted 4 lines
Quoted 2 lines

Tristan pointed out that this check was already in ocfs2_change_file_space(). So this patch should not be required. Am I missing something?

This patch is needed since the check is in ocfs2_change_file_space. But ocfs2_fallocate calls __ocfs2_change_file_space directly.

Right.

Quoted 2 lines

So maybe we should add the check here and remove the check in ocfs2_change_file_space?

Either way works for me.

	--Mark

--
Mark Fasheh

#6 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Quoted 16 lines · Mark Fasheh
Quoted 15 lines · Tao Ma
Quoted 14 lines · Sunil Mushran
Quoted 12 lines · Mark Fasheh

Review static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 8 lines · Li Dongyang
(quoted diff) +4 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 ? ? ? ? ? ? ? ? ? goto out_inode_unlock;
 ? ? ? ? ? }
 
+ if (!S_ISREG(inode->i_mode)) {
+ ? ? ? ? ret = -EINVAL;
+ ? ? ? ? goto out_inode_unlock;
+ }

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Tristan pointed out that this check was already in ocfs2_change_file_space(). So this patch should not be required. Am I missing something?

This patch is needed since the check is in ocfs2_change_file_space. But ocfs2_fallocate calls __ocfs2_change_file_space directly.

Right.

It seems it is not right:-) If I read the code correctly, although ocfs2_fallocate calls __ocfs2_change_file_space directly, however, ocfs2_fallocate is a file operation but not an inode operation, ocfs2_fallocate can only be registered when that file is a regular file, so this patch is not needed.

Thanks,
Jiaju

Quoted 16 lines
Quoted 4 lines

So maybe we should add the check here and remove the check in ocfs2_change_file_space?

Either way works for me.
? ? ? ?--Mark

--
Mark Fasheh

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel

#7 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Quoted 22 lines · Jiaju Zhang
Quoted 21 lines · Mark Fasheh
Quoted 19 lines · Tao Ma
Quoted 14 lines · Sunil Mushran
Quoted 5 lines · Mark Fasheh
Quoted 4 lines · Li Dongyang
(quoted diff) +0 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 ? ? ? ? ? ? ? ? ? goto out_inode_unlock;
 ? ? ? ? ? }
Quoted 7 lines

fs/ocfs2/file.c static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 4 lines
fs/ocfs2/file.c +4 -0
+ if (!S_ISREG(inode->i_mode)) {
+ ? ? ? ? ret = -EINVAL;
+ ? ? ? ? goto out_inode_unlock;
+ }

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

Quoted 2 lines

Tristan pointed out that this check was already in ocfs2_change_file_space(). So this patch should not be required. Am I missing something?

This patch is needed since the check is in ocfs2_change_file_space. But ocfs2_fallocate calls __ocfs2_change_file_space directly.

Quoted 7 lines
Quoted 1 line

Right.

It seems it is not right:-) If I read the code correctly, although ocfs2_fallocate calls __ocfs2_change_file_space directly, however, ocfs2_fallocate is a file operation but not an inode operation, ocfs2_fallocate can only be registered when that file is a regular file, so this patch is not needed.

Please review the code in fs/open.c:do_fallocate()

        /*
         * Let individual file system decide if it supports preallocation
         * for directories or not.
         */
        if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
                return -ENODEV;

So, sys_fallocate is leaving the decision up to the file system. Therefore I think your patch is needed after all :)

	--Mark

--
Mark Fasheh

#8 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Quoted 31 lines · Mark Fasheh
Quoted 22 lines · Jiaju Zhang
Quoted 21 lines · Mark Fasheh
Quoted 19 lines · Tao Ma
Quoted 14 lines · Sunil Mushran
Quoted 5 lines · Mark Fasheh
Quoted 4 lines · Li Dongyang
(quoted diff) +0 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 ? ? ? ? ? ? ? ? ? goto out_inode_unlock;
 ? ? ? ? ? }
Quoted 7 lines

Review static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 4 lines
(quoted diff) +4 -0
+ if (!S_ISREG(inode->i_mode)) {
+ ? ? ? ? ret = -EINVAL;
+ ? ? ? ? goto out_inode_unlock;
+ }

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

Quoted 2 lines

Tristan pointed out that this check was already in ocfs2_change_file_space(). So this patch should not be required. Am I missing something?

This patch is needed since the check is in ocfs2_change_file_space. But ocfs2_fallocate calls __ocfs2_change_file_space directly.

Quoted 7 lines
Quoted 1 line

Right.

It seems it is not right:-) If I read the code correctly, although ocfs2_fallocate calls __ocfs2_change_file_space directly, however, ocfs2_fallocate is a file operation but not an inode operation, ocfs2_fallocate can only be registered when that file is a regular file, so this patch is not needed.

Quoted 8 lines

Please review the code in fs/open.c:do_fallocate()

? ? ? ?/*
? ? ? ? * Let individual file system decide if it supports preallocation ? ? ? ? * for directories or not.
? ? ? ? */
? ? ? ?if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) ? ? ? ? ? ? ? ?return -ENODEV;

Yes.

Quoted 3 lines

So, sys_fallocate is leaving the decision up to the file system. Therefore I think your patch is needed after all :)

Maybe I haven't said more clear about this :-) I mean the check has been performed before, please see the code in fs/ocfs2/inode.c <<ocfs2_populate_inode>>:

322 switch (inode->i_mode & S_IFMT) {
323 case S_IFREG:
324 if (use_plocks)
325 inode->i_fop = &ocfs2_fops;
326 else
327 inode->i_fop = &ocfs2_fops_no_plocks;

 328                     inode->i_op = &ocfs2_file_iops;
 329                     i_size_write(inode, le64_to_cpu(fe->i_size));
 330                     break;
 331             case S_IFDIR:
 332                     inode->i_op = &ocfs2_dir_iops;
 333                     if (use_plocks)
 334                             inode->i_fop = &ocfs2_dops;
 335                     else
 336                             inode->i_fop = &ocfs2_dops_no_plocks;
 337                     i_size_write(inode, le64_to_cpu(fe->i_size));
 338                     OCFS2_I(inode)->ip_dir_lock_gen = 1;
 339                     break;

In line 323, we have checked the inode->i_mode, if it is S_IFREG, ocfs2_fops will be assigned to inode->i_fop; if it is S_IFDIR, ocfs2_dops should be assigned. And from the code in fs/ocfs2/file.c:

2626 const struct file_operations ocfs2_fops = {

2627         .llseek         = generic_file_llseek,
2628         .read           = do_sync_read,
2629         .write          = do_sync_write,
2630         .mmap           = ocfs2_mmap,
2631         .fsync          = ocfs2_sync_file,
2632         .release        = ocfs2_file_release,
2633         .open           = ocfs2_file_open,
2634         .aio_read       = ocfs2_file_aio_read,
2635         .aio_write      = ocfs2_file_aio_write,
2636         .unlocked_ioctl = ocfs2_ioctl,
2637 #ifdef CONFIG_COMPAT
2638         .compat_ioctl   = ocfs2_compat_ioctl,
2639 #endif
2640         .lock           = ocfs2_lock,
2641         .flock          = ocfs2_flock,
2642         .splice_read    = ocfs2_file_splice_read,
2643         .splice_write   = ocfs2_file_splice_write,
2644         .fallocate      = ocfs2_fallocate,
2645 };
2646
2647 const struct file_operations ocfs2_dops = {
2648         .llseek         = generic_file_llseek,
2649         .read           = generic_read_dir,
2650         .readdir        = ocfs2_readdir,
2651         .fsync          = ocfs2_sync_file,
2652         .release        = ocfs2_dir_release,
2653         .open           = ocfs2_dir_open,
2654         .unlocked_ioctl = ocfs2_ioctl,
2655 #ifdef CONFIG_COMPAT
2656         .compat_ioctl   = ocfs2_compat_ioctl,
2657 #endif
2658         .lock           = ocfs2_lock,
2659         .flock          = ocfs2_flock,

2660 };

ocfs2_fops has the fallocate field, but ocfs2_dops doesn't have it. So if the file is a directory, file->f_op->fallocate should be NULL, so in the function do_fallocate, it should have returned -EOPNOTSUPP already. So I think that check is not needed. Or am I missing something?:)

Thanks,
Jiaju

#9 lore raw

PATCH 1/1 Ocfs2: do not allow fallocate on dir file

Quoted 44 lines · Jiaju Zhang
Quoted 31 lines · Mark Fasheh
Quoted 22 lines · Jiaju Zhang
Quoted 21 lines · Mark Fasheh
Quoted 19 lines · Tao Ma
Quoted 14 lines · Sunil Mushran
Quoted 5 lines · Mark Fasheh
Quoted 4 lines · Li Dongyang
(quoted diff) +0 -0
@@ -1870,6 +1870,11 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 ? ? ? ? ? ? ? ? ? goto out_inode_unlock;
 ? ? ? ? ? }
Quoted 7 lines

Review static int __ocfs2_change_file_space(struct file *file, struct inode *inode, · line 1876

Quoted 4 lines
(quoted diff) +4 -0
+ if (!S_ISREG(inode->i_mode)) {
+ ? ? ? ? ret = -EINVAL;
+ ? ? ? ? goto out_inode_unlock;
+ }

You might want to move the check into ocfs2_fallocate to mirror what ocfs2_change_file_space() does. Otherwise, looks like a good catch.

Quoted 2 lines

Tristan pointed out that this check was already in ocfs2_change_file_space(). So this patch should not be required. Am I missing something?

This patch is needed since the check is in ocfs2_change_file_space. But ocfs2_fallocate calls __ocfs2_change_file_space directly.

Quoted 7 lines
Quoted 1 line

Right.

It seems it is not right:-) If I read the code correctly, although ocfs2_fallocate calls __ocfs2_change_file_space directly, however, ocfs2_fallocate is a file operation but not an inode operation, ocfs2_fallocate can only be registered when that file is a regular file, so this patch is not needed.

Quoted 8 lines

Please review the code in fs/open.c:do_fallocate()

? ? ? ?/*
? ? ? ? * Let individual file system decide if it supports preallocation ? ? ? ? * for directories or not.
? ? ? ? */
? ? ? ?if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) ? ? ? ? ? ? ? ?return -ENODEV;

Yes.

Quoted 1 line
Quoted 6 lines
Quoted 2 lines

So, sys_fallocate is leaving the decision up to the file system. Therefore I think your patch is needed after all :)

Maybe I haven't said more clear about this :-) I mean the check has been performed before, please see the code in fs/ocfs2/inode.c <<ocfs2_populate_inode>>:

Ahh ok. I see - we never set the fallocate callback on directory inodes in the first place. Disregard my previous e-mails then!

	--Mark

Mark Fasheh