Patchwork 9p: Be robust against paths without FS_IOC_GETVERSION

login
register
mail settings
Submitter Gabriel de Perthuis
Date May 8, 2013, 11:49 p.m.
Message ID <518AE484.9090204@gmail.com>
Download mbox | patch
Permalink /patch/242675/
State New
Headers show

Comments

Gabriel de Perthuis - May 8, 2013, 11:49 p.m.
The current implementation checked for supported filesystems at mount
time, but actual support depends on the path.  Don't error out when
finding unversioned paths.

This fix allows booting a linux kernel with the same / filesystem as the
host; otherwise the boot fails when mounting devtmpfs.

Signed-off-by: Gabriel de Perthuis <g2p.code@gmail.com>
---
 hw/9pfs/cofile.c | 4 ++++
 1 file changed, 4 insertions(+)
Aneesh Kumar K.V - May 10, 2013, 3:30 a.m.
Gabriel de Perthuis <g2p.code@gmail.com> writes:

> The current implementation checked for supported filesystems at mount
> time, but actual support depends on the path.  Don't error out when
> finding unversioned paths.

Can you elaborate this a bit ?

>
> This fix allows booting a linux kernel with the same / filesystem as the
> host; otherwise the boot fails when mounting devtmpfs.
>
> Signed-off-by: Gabriel de Perthuis <g2p.code@gmail.com>
> ---
>  hw/9pfs/cofile.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
> index 2efebf3..194c130 100644
> --- a/hw/9pfs/cofile.c
> +++ b/hw/9pfs/cofile.c
> @@ -36,10 +36,14 @@ int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
>                      err = -errno;
>                  }
>              });
>          v9fs_path_unlock(s);
>      }
> +    /* The ioctl may not be supported depending on the path */
> +    if (err == -ENOTTY) {
> +        err = 0;
> +    }

So you don't want to consider -ENOTTY as an error ? why ?

>      return err;
>  }
>
>  int v9fs_co_lstat(V9fsPDU *pdu, V9fsPath *path, struct stat *stbuf)
>  {
> -- 

-aneesh
Gabriel de Perthuis - May 10, 2013, 6:42 a.m.
Le 10/05/2013 05:30, Aneesh Kumar K.V a écrit :
> Gabriel de Perthuis <g2p.code@gmail.com> writes:
> 
>> The current implementation checked for supported filesystems at mount
>> time, but actual support depends on the path.  Don't error out when
>> finding unversioned paths.
> 
> Can you elaborate this a bit ?

ioctl support generally depends on the mount point.

You have a check that verifies the filesystem at the root of the source side
is ext4 or something else whitelisted to have generation support, but that's
not sufficient, the source may contain other mountpoints that don't support
the getversion ioctl.  When a path gives you ENOTTY, that means the ioctl
is not supported.

>> This fix allows booting a linux kernel with the same / filesystem as the
>> host; otherwise the boot fails when mounting devtmpfs.
>>
>> Signed-off-by: Gabriel de Perthuis <g2p.code@gmail.com>
>> ---
>>  hw/9pfs/cofile.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
>> index 2efebf3..194c130 100644
>> --- a/hw/9pfs/cofile.c
>> +++ b/hw/9pfs/cofile.c
>> @@ -36,10 +36,14 @@ int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
>>                      err = -errno;
>>                  }
>>              });
>>          v9fs_path_unlock(s);
>>      }
>> +    /* The ioctl may not be supported depending on the path */
>> +    if (err == -ENOTTY) {
>> +        err = 0;
>> +    }
> 
> So you don't want to consider -ENOTTY as an error ? why ?

ENOTTY means the ioctl is not supported.
That should be handled the same as if the source-side root didn't have a
whitelisted filesystem.  That means we need to return 0 in both cases.

>>      return err;
>>  }
>>
>>  int v9fs_co_lstat(V9fsPDU *pdu, V9fsPath *path, struct stat *stbuf)
>>  {
>> -- 
> 
> -aneesh
Aneesh Kumar K.V - May 10, 2013, 10:38 a.m.
Gabriel de Perthuis <g2p.code@gmail.com> writes:

> Le 10/05/2013 05:30, Aneesh Kumar K.V a écrit :
>> Gabriel de Perthuis <g2p.code@gmail.com> writes:
>> 
>>> The current implementation checked for supported filesystems at mount
>>> time, but actual support depends on the path.  Don't error out when
>>> finding unversioned paths.
>> 
>> Can you elaborate this a bit ?
>
> ioctl support generally depends on the mount point.
>
> You have a check that verifies the filesystem at the root of the source side
> is ext4 or something else whitelisted to have generation support, but that's
> not sufficient, the source may contain other mountpoints that don't support
> the getversion ioctl.  When a path gives you ENOTTY, that means the ioctl
> is not supported.

May be we can update the commit message to be more clear. ie, currently
the st_gen support is decided based on whether export file system
support ioc_getversion ioctl. But we could have other file system mounted further down
in the exported path that doesn't support ioc_getversion. So instead of
failing stat request, we do the same we did for the export path. If we
find the ioctl not supported we silently ignore the error. This results in us
returning st_gen value 0 which should be ok 

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

>
>>> This fix allows booting a linux kernel with the same / filesystem as the
>>> host; otherwise the boot fails when mounting devtmpfs.
>>>
>>> Signed-off-by: Gabriel de Perthuis <g2p.code@gmail.com>
>>> ---
>>>  hw/9pfs/cofile.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
>>> index 2efebf3..194c130 100644
>>> --- a/hw/9pfs/cofile.c
>>> +++ b/hw/9pfs/cofile.c
>>> @@ -36,10 +36,14 @@ int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
>>>                      err = -errno;
>>>                  }
>>>              });
>>>          v9fs_path_unlock(s);
>>>      }
>>> +    /* The ioctl may not be supported depending on the path */
>>> +    if (err == -ENOTTY) {
>>> +        err = 0;
>>> +    }
>> 
>> So you don't want to consider -ENOTTY as an error ? why ?
>
> ENOTTY means the ioctl is not supported.
> That should be handled the same as if the source-side root didn't have a
> whitelisted filesystem.  That means we need to return 0 in both cases.
>
>>>      return err;
>>>  }
>>>
>>>  int v9fs_co_lstat(V9fsPDU *pdu, V9fsPath *path, struct stat *stbuf)
>>>  {
>>> -- 
>> 
>> -aneesh

Patch

diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
index 2efebf3..194c130 100644
--- a/hw/9pfs/cofile.c
+++ b/hw/9pfs/cofile.c
@@ -36,10 +36,14 @@  int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
                     err = -errno;
                 }
             });
         v9fs_path_unlock(s);
     }
+    /* The ioctl may not be supported depending on the path */
+    if (err == -ENOTTY) {
+        err = 0;
+    }
     return err;
 }
 
 int v9fs_co_lstat(V9fsPDU *pdu, V9fsPath *path, struct stat *stbuf)
 {