[PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Austin S. Hemmelgarn]

Currently, open_ctree_fs_info will open whatever path you pass it and
try to interpret it as a BTRFS filesystem.  While this is not
nessecarily dangerous (except possibly if done on a character device),
it does result in some rather cryptic and non-sensical error messages
when trying to run certain commands in ways they weren't intended to be
run.  Add a check using stat(2) to verify that the path we've been
passed is in fact a regular file or a block device.

This causes the following commands to provide a helpful error message
when run on a FIFO, directory, character device, or socket:
    * btrfs check
    * btrfs restore
    * btrfs-image
    * btrfs-find-root
    * btrfs-debug-tree

Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
---
This has been build and runtime tested on an x86-64 system with glibc.
It has been build tested on x86-64 with uclibc.
It has not been tested on Android or with musl, although it should work
there also.

There are other tools that have similarly bad behavior when called
incorrectly (btrfs rescue immediately comes to mind), but they don't
use open_ctree_fs_info, so this doesn't affect them.  I may do followup
patches to fix those too if I have the time.

open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not
at all sure what exactly is going on there, and btrfs filesystem appears
from my testing to behave exactly the same with this change, so I don't
think this will have any effect on any of the btrfs filesystem commands.

 disk-io.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/disk-io.c b/disk-io.c
index e520d80..d35153d 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const char *filename,
 	int fp;
 	struct btrfs_fs_info *info;
 	int oflags = O_CREAT | O_RDWR;
+	struct stat sb;
+
+	stat(filename, &sb);
+	if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode & S_IFMT) == S_IFBLK))) {
+		fprintf (stderr, "%s is not a regular file or block device\n", filename);
+		return NULL;
+	}
 
 	if (!(flags & OPEN_CTREE_WRITES))
 		oflags = O_RDONLY;
-- 
2.7.3

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Duncan]

Austin S. Hemmelgarn posted on Wed, 16 Mar 2016 11:26:11 -0400 as
excerpted:

> Currently, open_ctree_fs_info will open whatever path you pass it and
> try to interpret it as a BTRFS filesystem.  While this is not
> nessecarily dangerous (except possibly if done on a character device),
> it does result in some rather cryptic and non-sensical error messages
> when trying to run certain commands in ways they weren't intended to be
> run.  Add a check using stat(2) to verify that the path we've been
> passed is in fact a regular file or a block device.
> 
> This causes the following commands to provide a helpful error message
> when run on a FIFO, directory, character device, or socket:

Umm... what about a symlink?  A symlink isn't a block device or regular 
file, but obviously with udev, symlinks to block devices /better/ work.  
Do you dereference symlinks before doing this check?


-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Austin S. Hemmelgarn]

On 2016-03-17 04:58, Duncan wrote:
> Austin S. Hemmelgarn posted on Wed, 16 Mar 2016 11:26:11 -0400 as
> excerpted:
>
>> Currently, open_ctree_fs_info will open whatever path you pass it and
>> try to interpret it as a BTRFS filesystem.  While this is not
>> nessecarily dangerous (except possibly if done on a character device),
>> it does result in some rather cryptic and non-sensical error messages
>> when trying to run certain commands in ways they weren't intended to be
>> run.  Add a check using stat(2) to verify that the path we've been
>> passed is in fact a regular file or a block device.
>>
>> This causes the following commands to provide a helpful error message
>> when run on a FIFO, directory, character device, or socket:
>
> Umm... what about a symlink?  A symlink isn't a block device or regular
> file, but obviously with udev, symlinks to block devices /better/ work.
> Do you dereference symlinks before doing this check?
>
>
I'm using stat(2) instead of lstat(2), and stat(2) dereferences symlinks 
just like open(2) does, so I shouldn't have to dereference them 
manually.  That said, I did check that it works with symlinks to both 
block devices and regular files, and everything appears to work correctly.

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Qu Wenruo]

Austin S. Hemmelgarn wrote on 2016/03/16 11:26 -0400:
> Currently, open_ctree_fs_info will open whatever path you pass it and
> try to interpret it as a BTRFS filesystem.  While this is not
> nessecarily dangerous (except possibly if done on a character device),
> it does result in some rather cryptic and non-sensical error messages
> when trying to run certain commands in ways they weren't intended to be
> run.  Add a check using stat(2) to verify that the path we've been
> passed is in fact a regular file or a block device.
>
> This causes the following commands to provide a helpful error message
> when run on a FIFO, directory, character device, or socket:
>      * btrfs check
>      * btrfs restore
>      * btrfs-image
>      * btrfs-find-root
>      * btrfs-debug-tree
>
> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
> ---
> This has been build and runtime tested on an x86-64 system with glibc.
> It has been build tested on x86-64 with uclibc.
> It has not been tested on Android or with musl, although it should work
> there also.
>
> There are other tools that have similarly bad behavior when called
> incorrectly (btrfs rescue immediately comes to mind), but they don't
> use open_ctree_fs_info, so this doesn't affect them.  I may do followup
> patches to fix those too if I have the time.
>
> open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not
> at all sure what exactly is going on there, and btrfs filesystem appears
> from my testing to behave exactly the same with this change, so I don't
> think this will have any effect on any of the btrfs filesystem commands.
>
>   disk-io.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/disk-io.c b/disk-io.c
> index e520d80..d35153d 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const char *filename,
>   	int fp;
>   	struct btrfs_fs_info *info;
>   	int oflags = O_CREAT | O_RDWR;
> +	struct stat sb;
> +
> +	stat(filename, &sb);
> +	if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode & S_IFMT) == S_IFBLK))) {
> +		fprintf (stderr, "%s is not a regular file or block device\n", filename);
> +		return NULL;
> +	}

This one seems to be too restrict.

I prefer to block char/pipe/dir and some other obvious wrong ones other 
than only allowing regular and block ones.

Thanks,
Qu
>
>   	if (!(flags & OPEN_CTREE_WRITES))
>   		oflags = O_RDONLY;
>

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Austin S. Hemmelgarn]

On 2016-03-17 05:04, Qu Wenruo wrote:
>
>
> Austin S. Hemmelgarn wrote on 2016/03/16 11:26 -0400:
>> Currently, open_ctree_fs_info will open whatever path you pass it and
>> try to interpret it as a BTRFS filesystem.  While this is not
>> nessecarily dangerous (except possibly if done on a character device),
>> it does result in some rather cryptic and non-sensical error messages
>> when trying to run certain commands in ways they weren't intended to be
>> run.  Add a check using stat(2) to verify that the path we've been
>> passed is in fact a regular file or a block device.
>>
>> This causes the following commands to provide a helpful error message
>> when run on a FIFO, directory, character device, or socket:
>>      * btrfs check
>>      * btrfs restore
>>      * btrfs-image
>>      * btrfs-find-root
>>      * btrfs-debug-tree
>>
>> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
>> ---
>> This has been build and runtime tested on an x86-64 system with glibc.
>> It has been build tested on x86-64 with uclibc.
>> It has not been tested on Android or with musl, although it should work
>> there also.
>>
>> There are other tools that have similarly bad behavior when called
>> incorrectly (btrfs rescue immediately comes to mind), but they don't
>> use open_ctree_fs_info, so this doesn't affect them.  I may do followup
>> patches to fix those too if I have the time.
>>
>> open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not
>> at all sure what exactly is going on there, and btrfs filesystem appears
>> from my testing to behave exactly the same with this change, so I don't
>> think this will have any effect on any of the btrfs filesystem commands.
>>
>>   disk-io.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/disk-io.c b/disk-io.c
>> index e520d80..d35153d 100644
>> --- a/disk-io.c
>> +++ b/disk-io.c
>> @@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const
>> char *filename,
>>       int fp;
>>       struct btrfs_fs_info *info;
>>       int oflags = O_CREAT | O_RDWR;
>> +    struct stat sb;
>> +
>> +    stat(filename, &sb);
>> +    if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode &
>> S_IFMT) == S_IFBLK))) {
>> +        fprintf (stderr, "%s is not a regular file or block
>> device\n", filename);
>> +        return NULL;
>> +    }
>
> This one seems to be too restrict.
>
> I prefer to block char/pipe/dir and some other obvious wrong ones other
> than only allowing regular and block ones.
Running against a directory gives a cryptic error about the superblock 
having bad info.  Running against a pipe is nonsensical, as it can't 
contain a filesystem.  Running against a character device is potentially 
dangerous (read operations are not guaranteed to be idempotent on 
character devices, depending on what hardware it is connected to, you 
could cause all kinds of odd things to happen).

Everything this function gets called on is trying to get info from a 
unmounted filesystem image, which means that it only makes sense to try 
to parse things that can contain a unmounted filesystem image.
>
> Thanks,
> Qu
>>
>>       if (!(flags & OPEN_CTREE_WRITES))
>>           oflags = O_RDONLY;
>>
>
>

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Qu Wenruo]

Austin S. Hemmelgarn wrote on 2016/03/17 07:22 -0400:
> On 2016-03-17 05:04, Qu Wenruo wrote:
>>
>>
>> Austin S. Hemmelgarn wrote on 2016/03/16 11:26 -0400:
>>> Currently, open_ctree_fs_info will open whatever path you pass it and
>>> try to interpret it as a BTRFS filesystem.  While this is not
>>> nessecarily dangerous (except possibly if done on a character device),
>>> it does result in some rather cryptic and non-sensical error messages
>>> when trying to run certain commands in ways they weren't intended to be
>>> run.  Add a check using stat(2) to verify that the path we've been
>>> passed is in fact a regular file or a block device.
>>>
>>> This causes the following commands to provide a helpful error message
>>> when run on a FIFO, directory, character device, or socket:
>>>      * btrfs check
>>>      * btrfs restore
>>>      * btrfs-image
>>>      * btrfs-find-root
>>>      * btrfs-debug-tree
>>>
>>> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
>>> ---
>>> This has been build and runtime tested on an x86-64 system with glibc.
>>> It has been build tested on x86-64 with uclibc.
>>> It has not been tested on Android or with musl, although it should work
>>> there also.
>>>
>>> There are other tools that have similarly bad behavior when called
>>> incorrectly (btrfs rescue immediately comes to mind), but they don't
>>> use open_ctree_fs_info, so this doesn't affect them.  I may do followup
>>> patches to fix those too if I have the time.
>>>
>>> open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not
>>> at all sure what exactly is going on there, and btrfs filesystem appears
>>> from my testing to behave exactly the same with this change, so I don't
>>> think this will have any effect on any of the btrfs filesystem commands.
>>>
>>>   disk-io.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/disk-io.c b/disk-io.c
>>> index e520d80..d35153d 100644
>>> --- a/disk-io.c
>>> +++ b/disk-io.c
>>> @@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const
>>> char *filename,
>>>       int fp;
>>>       struct btrfs_fs_info *info;
>>>       int oflags = O_CREAT | O_RDWR;
>>> +    struct stat sb;
>>> +
>>> +    stat(filename, &sb);
>>> +    if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode &
>>> S_IFMT) == S_IFBLK))) {
>>> +        fprintf (stderr, "%s is not a regular file or block
>>> device\n", filename);
>>> +        return NULL;
>>> +    }
>>
>> This one seems to be too restrict.
>>
>> I prefer to block char/pipe/dir and some other obvious wrong ones other
>> than only allowing regular and block ones.
> Running against a directory gives a cryptic error about the superblock
> having bad info.  Running against a pipe is nonsensical, as it can't
> contain a filesystem.  Running against a character device is potentially
> dangerous (read operations are not guaranteed to be idempotent on
> character devices, depending on what hardware it is connected to, you
> could cause all kinds of odd things to happen).
>
> Everything this function gets called on is trying to get info from a
> unmounted filesystem image, which means that it only makes sense to try
> to parse things that can contain a unmounted filesystem image.

Yes, I understand what you are doing.

Just as I alreayd mentioned, the problem is, your current patch only 
allowing regular and block device and will block valid soft link.
Just as Duncan mentioned, soft link should be allowed too.

I mean to *block/prevent* char/pipe/dir instead of *only allowing* 
regular/block device.

Thanks,
Qu
>>
>> Thanks,
>> Qu
>>>
>>>       if (!(flags & OPEN_CTREE_WRITES))
>>>           oflags = O_RDONLY;
>>>
>>
>>
>
>
>

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Austin S. Hemmelgarn]

On 2016-03-17 20:38, Qu Wenruo wrote:
>
>
> Austin S. Hemmelgarn wrote on 2016/03/17 07:22 -0400:
>> On 2016-03-17 05:04, Qu Wenruo wrote:
>>>
>>>
>>> Austin S. Hemmelgarn wrote on 2016/03/16 11:26 -0400:
>>>> Currently, open_ctree_fs_info will open whatever path you pass it and
>>>> try to interpret it as a BTRFS filesystem.  While this is not
>>>> nessecarily dangerous (except possibly if done on a character device),
>>>> it does result in some rather cryptic and non-sensical error messages
>>>> when trying to run certain commands in ways they weren't intended to be
>>>> run.  Add a check using stat(2) to verify that the path we've been
>>>> passed is in fact a regular file or a block device.
>>>>
>>>> This causes the following commands to provide a helpful error message
>>>> when run on a FIFO, directory, character device, or socket:
>>>>      * btrfs check
>>>>      * btrfs restore
>>>>      * btrfs-image
>>>>      * btrfs-find-root
>>>>      * btrfs-debug-tree
>>>>
>>>> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
>>>> ---
>>>> This has been build and runtime tested on an x86-64 system with glibc.
>>>> It has been build tested on x86-64 with uclibc.
>>>> It has not been tested on Android or with musl, although it should work
>>>> there also.
>>>>
>>>> There are other tools that have similarly bad behavior when called
>>>> incorrectly (btrfs rescue immediately comes to mind), but they don't
>>>> use open_ctree_fs_info, so this doesn't affect them.  I may do followup
>>>> patches to fix those too if I have the time.
>>>>
>>>> open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not
>>>> at all sure what exactly is going on there, and btrfs filesystem
>>>> appears
>>>> from my testing to behave exactly the same with this change, so I don't
>>>> think this will have any effect on any of the btrfs filesystem
>>>> commands.
>>>>
>>>>   disk-io.c | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/disk-io.c b/disk-io.c
>>>> index e520d80..d35153d 100644
>>>> --- a/disk-io.c
>>>> +++ b/disk-io.c
>>>> @@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const
>>>> char *filename,
>>>>       int fp;
>>>>       struct btrfs_fs_info *info;
>>>>       int oflags = O_CREAT | O_RDWR;
>>>> +    struct stat sb;
>>>> +
>>>> +    stat(filename, &sb);
>>>> +    if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode &
>>>> S_IFMT) == S_IFBLK))) {
>>>> +        fprintf (stderr, "%s is not a regular file or block
>>>> device\n", filename);
>>>> +        return NULL;
>>>> +    }
>>>
>>> This one seems to be too restrict.
>>>
>>> I prefer to block char/pipe/dir and some other obvious wrong ones other
>>> than only allowing regular and block ones.
>> Running against a directory gives a cryptic error about the superblock
>> having bad info.  Running against a pipe is nonsensical, as it can't
>> contain a filesystem.  Running against a character device is potentially
>> dangerous (read operations are not guaranteed to be idempotent on
>> character devices, depending on what hardware it is connected to, you
>> could cause all kinds of odd things to happen).
>>
>> Everything this function gets called on is trying to get info from a
>> unmounted filesystem image, which means that it only makes sense to try
>> to parse things that can contain a unmounted filesystem image.
>
> Yes, I understand what you are doing.
>
> Just as I alreayd mentioned, the problem is, your current patch only
> allowing regular and block device and will block valid soft link.
> Just as Duncan mentioned, soft link should be allowed too.
Symbolic links are allowed though.  stat(2) follows symlinks and returns 
information on their target, not the link itself.  I also tested it on 
symlinks, and it still works to run `btrfs check` on a link to a block 
device containing a BTRFS filesystem
>
> I mean to *block/prevent* char/pipe/dir instead of *only allowing*
> regular/block device.
I apologize for the misunderstanding, I misread your original message.

As to blacklisting invalid types instead of whitelisting valid ones, I'd 
personally would prefer whitelisting in this particular case, as it 
results in both a smaller conditional, and makes the intent of only 
allowing things that can contain a BTRFS filesystem  more clear (at 
least, it does to me).
>
> Thanks,
> Qu
>>>
>>> Thanks,
>>> Qu
>>>>
>>>>       if (!(flags & OPEN_CTREE_WRITES))
>>>>           oflags = O_RDONLY;
>>>>
>>>
>>>
>>
>>
>>
>
>

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[David Sterba]

On Fri, Mar 18, 2016 at 07:17:21AM -0400, Austin S. Hemmelgarn wrote:
> >>> This one seems to be too restrict.
> >>>
> >>> I prefer to block char/pipe/dir and some other obvious wrong ones other
> >>> than only allowing regular and block ones.
> >> Running against a directory gives a cryptic error about the superblock
> >> having bad info.  Running against a pipe is nonsensical, as it can't
> >> contain a filesystem.  Running against a character device is potentially
> >> dangerous (read operations are not guaranteed to be idempotent on
> >> character devices, depending on what hardware it is connected to, you
> >> could cause all kinds of odd things to happen).
> >>
> >> Everything this function gets called on is trying to get info from a
> >> unmounted filesystem image, which means that it only makes sense to try
> >> to parse things that can contain a unmounted filesystem image.
> >
> > Yes, I understand what you are doing.
> >
> > Just as I alreayd mentioned, the problem is, your current patch only
> > allowing regular and block device and will block valid soft link.
> > Just as Duncan mentioned, soft link should be allowed too.
> Symbolic links are allowed though.  stat(2) follows symlinks and returns 
> information on their target, not the link itself.  I also tested it on 
> symlinks, and it still works to run `btrfs check` on a link to a block 
> device containing a BTRFS filesystem
> >
> > I mean to *block/prevent* char/pipe/dir instead of *only allowing*
> > regular/block device.
> I apologize for the misunderstanding, I misread your original message.
> 
> As to blacklisting invalid types instead of whitelisting valid ones, I'd 
> personally would prefer whitelisting in this particular case, as it 
> results in both a smaller conditional, and makes the intent of only 
> allowing things that can contain a BTRFS filesystem  more clear (at 
> least, it does to me).

I agree with your reasoning.

Re: [PATCH] btrfs-progs: add stat check in open_ctree_fs_info

[Qu Wenruo]

Austin S. Hemmelgarn wrote on 2016/03/18 07:17 -0400:
> On 2016-03-17 20:38, Qu Wenruo wrote:
>>
>>
>> Austin S. Hemmelgarn wrote on 2016/03/17 07:22 -0400:
>>> On 2016-03-17 05:04, Qu Wenruo wrote:
>>>>
>>>>
>>>> Austin S. Hemmelgarn wrote on 2016/03/16 11:26 -0400:
>>>>> Currently, open_ctree_fs_info will open whatever path you pass it and
>>>>> try to interpret it as a BTRFS filesystem.  While this is not
>>>>> nessecarily dangerous (except possibly if done on a character device),
>>>>> it does result in some rather cryptic and non-sensical error messages
>>>>> when trying to run certain commands in ways they weren't intended
>>>>> to be
>>>>> run.  Add a check using stat(2) to verify that the path we've been
>>>>> passed is in fact a regular file or a block device.
>>>>>
>>>>> This causes the following commands to provide a helpful error message
>>>>> when run on a FIFO, directory, character device, or socket:
>>>>>      * btrfs check
>>>>>      * btrfs restore
>>>>>      * btrfs-image
>>>>>      * btrfs-find-root
>>>>>      * btrfs-debug-tree
>>>>>
>>>>> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
>>>>> ---
>>>>> This has been build and runtime tested on an x86-64 system with glibc.
>>>>> It has been build tested on x86-64 with uclibc.
>>>>> It has not been tested on Android or with musl, although it should
>>>>> work
>>>>> there also.
>>>>>
>>>>> There are other tools that have similarly bad behavior when called
>>>>> incorrectly (btrfs rescue immediately comes to mind), but they don't
>>>>> use open_ctree_fs_info, so this doesn't affect them.  I may do
>>>>> followup
>>>>> patches to fix those too if I have the time.
>>>>>
>>>>> open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not
>>>>> at all sure what exactly is going on there, and btrfs filesystem
>>>>> appears
>>>>> from my testing to behave exactly the same with this change, so I
>>>>> don't
>>>>> think this will have any effect on any of the btrfs filesystem
>>>>> commands.
>>>>>
>>>>>   disk-io.c | 7 +++++++
>>>>>   1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/disk-io.c b/disk-io.c
>>>>> index e520d80..d35153d 100644
>>>>> --- a/disk-io.c
>>>>> +++ b/disk-io.c
>>>>> @@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const
>>>>> char *filename,
>>>>>       int fp;
>>>>>       struct btrfs_fs_info *info;
>>>>>       int oflags = O_CREAT | O_RDWR;
>>>>> +    struct stat sb;
>>>>> +
>>>>> +    stat(filename, &sb);
>>>>> +    if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode &
>>>>> S_IFMT) == S_IFBLK))) {
>>>>> +        fprintf (stderr, "%s is not a regular file or block
>>>>> device\n", filename);
>>>>> +        return NULL;
>>>>> +    }
>>>>
>>>> This one seems to be too restrict.
>>>>
>>>> I prefer to block char/pipe/dir and some other obvious wrong ones other
>>>> than only allowing regular and block ones.
>>> Running against a directory gives a cryptic error about the superblock
>>> having bad info.  Running against a pipe is nonsensical, as it can't
>>> contain a filesystem.  Running against a character device is potentially
>>> dangerous (read operations are not guaranteed to be idempotent on
>>> character devices, depending on what hardware it is connected to, you
>>> could cause all kinds of odd things to happen).
>>>
>>> Everything this function gets called on is trying to get info from a
>>> unmounted filesystem image, which means that it only makes sense to try
>>> to parse things that can contain a unmounted filesystem image.
>>
>> Yes, I understand what you are doing.
>>
>> Just as I alreayd mentioned, the problem is, your current patch only
>> allowing regular and block device and will block valid soft link.
>> Just as Duncan mentioned, soft link should be allowed too.
> Symbolic links are allowed though.  stat(2) follows symlinks and returns
> information on their target, not the link itself.  I also tested it on
> symlinks, and it still works to run `btrfs check` on a link to a block
> device containing a BTRFS filesystem

Then I'm completely OK with current implementation.

Thanks,
Qu
>>
>> I mean to *block/prevent* char/pipe/dir instead of *only allowing*
>> regular/block device.
> I apologize for the misunderstanding, I misread your original message.
>
> As to blacklisting invalid types instead of whitelisting valid ones, I'd
> personally would prefer whitelisting in this particular case, as it
> results in both a smaller conditional, and makes the intent of only
> allowing things that can contain a BTRFS filesystem  more clear (at
> least, it does to me).
>>
>> Thanks,
>> Qu
>>>>
>>>> Thanks,
>>>> Qu
>>>>>
>>>>>       if (!(flags & OPEN_CTREE_WRITES))
>>>>>           oflags = O_RDONLY;
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>