[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[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 <lidongyang@novell.com>
---
 fs/ocfs2/file.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index a665195..0c68a61 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -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;
-- 
1.7.1

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Tristan Ye]

Hi Dongyang,

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



Li Dongyang wrote:
> 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 <lidongyang@novell.com>
> ---
>  fs/ocfs2/file.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index a665195..0c68a61 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -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;

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Mark Fasheh]

On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
> 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 <lidongyang@novell.com>
> ---
>  fs/ocfs2/file.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index a665195..0c68a61 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -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

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Sunil Mushran]

On 03/09/2011 03:01 PM, Mark Fasheh wrote:
> On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
>> @@ -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.
>

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

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Tao Ma]

On 03/10/2011 10:27 AM, Sunil Mushran wrote:
> On 03/09/2011 03:01 PM, Mark Fasheh wrote:
>> On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
>>> @@ -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.
>>
> 
> 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

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Mark Fasheh]

On Thu, Mar 10, 2011 at 10:46:33AM +0800, Tao Ma wrote:
> On 03/10/2011 10:27 AM, Sunil Mushran wrote:
> > On 03/09/2011 03:01 PM, Mark Fasheh wrote:
> >> On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
> >>> @@ -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.
> >>
> > 
> > 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.


> 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] [PATCH] Ocfs2: do not allow fallocate on dir file

[Jiaju Zhang]

On Thu, Mar 10, 2011 at 12:25 PM, Mark Fasheh <mfasheh@suse.com> wrote:
> On Thu, Mar 10, 2011 at 10:46:33AM +0800, Tao Ma wrote:
>> On 03/10/2011 10:27 AM, Sunil Mushran wrote:
>> > On 03/09/2011 03:01 PM, Mark Fasheh wrote:
>> >> On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
>> >>> @@ -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.
>> >>
>> >
>> > 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

>
>
>> 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
>

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Mark Fasheh]

On Thu, Mar 10, 2011 at 02:00:33PM +0800, Jiaju Zhang wrote:
> On Thu, Mar 10, 2011 at 12:25 PM, Mark Fasheh <mfasheh@suse.com> wrote:
> > On Thu, Mar 10, 2011 at 10:46:33AM +0800, Tao Ma wrote:
> >> On 03/10/2011 10:27 AM, Sunil Mushran wrote:
> >> > On 03/09/2011 03:01 PM, Mark Fasheh wrote:
> >> >> On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
> >> >>> @@ -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.
> >> >>
> >> >
> >> > 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.

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

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Jiaju Zhang]

On Fri, Mar 11, 2011 at 12:54 AM, Mark Fasheh <mfasheh@suse.com> wrote:
> On Thu, Mar 10, 2011 at 02:00:33PM +0800, Jiaju Zhang wrote:
>> On Thu, Mar 10, 2011 at 12:25 PM, Mark Fasheh <mfasheh@suse.com> wrote:
>> > On Thu, Mar 10, 2011 at 10:46:33AM +0800, Tao Ma wrote:
>> >> On 03/10/2011 10:27 AM, Sunil Mushran wrote:
>> >> > On 03/09/2011 03:01 PM, Mark Fasheh wrote:
>> >> >> On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
>> >> >>> @@ -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.
>> >> >>
>> >> >
>> >> > 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.
>
> 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.

>
> 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

[Ocfs2-devel] [PATCH] Ocfs2: do not allow fallocate on dir file

[Mark Fasheh]

On Fri, Mar 11, 2011 at 02:33:41AM +0800, Jiaju Zhang wrote:
> On Fri, Mar 11, 2011 at 12:54 AM, Mark Fasheh <mfasheh@suse.com> wrote:
> > On Thu, Mar 10, 2011 at 02:00:33PM +0800, Jiaju Zhang wrote:
> >> On Thu, Mar 10, 2011 at 12:25 PM, Mark Fasheh <mfasheh@suse.com> wrote:
> >> > On Thu, Mar 10, 2011 at 10:46:33AM +0800, Tao Ma wrote:
> >> >> On 03/10/2011 10:27 AM, Sunil Mushran wrote:
> >> >> > On 03/09/2011 03:01 PM, Mark Fasheh wrote:
> >> >> >> On Fri, Mar 04, 2011 at 03:10:33PM +0800, Li Dongyang wrote:
> >> >> >>> @@ -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.
> >> >> >>
> >> >> >
> >> >> > 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.
> >
> > 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.
> 
> >
> > 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