Ocfs2 devel - Mar 2011 - [PATCH] 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 <lidongyang at 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

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

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

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?

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

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

On Thu, Mar 10, 2011 at 12:25 PM, Mark Fasheh <mfasheh at 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
>

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