[PATCH] filesetup: do not ask O_RDWR for read-only workload

[PATCH] filesetup: do not ask O_RDWR for read-only workload

[Kanchan Joshi]

Use O_RDONLY flag when read is requested on char-type files.

Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---

Before this patch: unexpected permission-denial for unprivileged-user.

$ ls -l /dev/ng0n1
cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1

$ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
-size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
4096B-4096B, ioengine=io_uring_cmd, iodepth=1
fio-3.33-71-g7d7a
Starting 1 process
fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
error=Permission denied

 filesetup.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/filesetup.c b/filesetup.c
index 1d3cc5ad..d77b8ba4 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -768,10 +768,7 @@ open_again:
 		else
 			from_hash = file_lookup_open(f, flags);
 	} else if (td_read(td)) {
-		if (f->filetype == FIO_TYPE_CHAR && !read_only)
-			flags |= O_RDWR;
-		else
-			flags |= O_RDONLY;
+		flags |= O_RDONLY;
 
 		if (is_std)
 			f->fd = dup(STDIN_FILENO);
-- 
2.25.1

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Jens Axboe]

On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
> 
> Use O_RDONLY flag when read is requested on char-type files.
> 
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
> 
> Before this patch: unexpected permission-denial for unprivileged-user.
> 
> $ ls -l /dev/ng0n1
> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
> 
> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
> fio-3.33-71-g7d7a
> Starting 1 process
> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
> error=Permission denied
> 
> filesetup.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/filesetup.c b/filesetup.c
> index 1d3cc5ad..d77b8ba4 100644
> --- a/filesetup.c
> +++ b/filesetup.c
> @@ -768,10 +768,7 @@ open_again:
>        else
>            from_hash = file_lookup_open(f, flags);
>    } else if (td_read(td)) {
> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
> -            flags |= O_RDWR;
> -        else
> -            flags |= O_RDONLY;
> +        flags |= O_RDONLY;

This will break sg like interfaces, where a read is done by writing the command to the char device. 

— 
Jens Axboe

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Vincent Fu]

On 2/3/23 07:46, Jens Axboe wrote:
> On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>
>> Use O_RDONLY flag when read is requested on char-type files.
>>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> ---
>>
>> Before this patch: unexpected permission-denial for unprivileged-user.
>>
>> $ ls -l /dev/ng0n1
>> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>
>> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>> fio-3.33-71-g7d7a
>> Starting 1 process
>> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>> error=Permission denied
>>
>> filesetup.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/filesetup.c b/filesetup.c
>> index 1d3cc5ad..d77b8ba4 100644
>> --- a/filesetup.c
>> +++ b/filesetup.c
>> @@ -768,10 +768,7 @@ open_again:
>>         else
>>             from_hash = file_lookup_open(f, flags);
>>     } else if (td_read(td)) {
>> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>> -            flags |= O_RDWR;
>> -        else
>> -            flags |= O_RDONLY;
>> +        flags |= O_RDONLY;
> 
> This will break sg like interfaces, where a read is done by writing the command to the char device.
> 
> —
> Jens Axboe
> 

Kanchan, does it work if you run fio with the --readonly option?

Vincent

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Jens Axboe]

On 2/3/23 7:08?AM, Vincent Fu wrote:
> On 2/3/23 07:46, Jens Axboe wrote:
>> On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>>
>>> ?Use O_RDONLY flag when read is requested on char-type files.
>>>
>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>> ---
>>>
>>> Before this patch: unexpected permission-denial for unprivileged-user.
>>>
>>> $ ls -l /dev/ng0n1
>>> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>>
>>> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>>> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>>> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>>> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>>> fio-3.33-71-g7d7a
>>> Starting 1 process
>>> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>>> error=Permission denied
>>>
>>> filesetup.c | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/filesetup.c b/filesetup.c
>>> index 1d3cc5ad..d77b8ba4 100644
>>> --- a/filesetup.c
>>> +++ b/filesetup.c
>>> @@ -768,10 +768,7 @@ open_again:
>>>         else
>>>             from_hash = file_lookup_open(f, flags);
>>>     } else if (td_read(td)) {
>>> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>>> -            flags |= O_RDWR;
>>> -        else
>>> -            flags |= O_RDONLY;
>>> +        flags |= O_RDONLY;
>>
>> This will break sg like interfaces, where a read is done by writing the command to the char device.
>>
>> ?
>> Jens Axboe
>>
> 
> Kanchan, does it work if you run fio with the --readonly option?

I think we should just make it work. Conceptually, the patch is obviously
fine, it just happens to break some oddball cases like sg/bsg. But maybe
we just add a fio ioengine flag for that, like FIO_RO_NEEDS_RW_OPEN or
something, where the engine can tell us if it needs a writeable open
even or a read-only workload.

Then generic_file_open() can use O_RDONLY for td_read(), except if the
engine has FIO_RO_NEEDS_RW_OPEN set.

-- 
Jens Axboe

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Vincent Fu]

On 2/3/23 09:14, Jens Axboe wrote:
> On 2/3/23 7:08?AM, Vincent Fu wrote:
>> On 2/3/23 07:46, Jens Axboe wrote:
>>> On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>>>
>>>> ?Use O_RDONLY flag when read is requested on char-type files.
>>>>
>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>> ---
>>>>
>>>> Before this patch: unexpected permission-denial for unprivileged-user.
>>>>
>>>> $ ls -l /dev/ng0n1
>>>> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>>>
>>>> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>>>> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>>>> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>>>> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>>>> fio-3.33-71-g7d7a
>>>> Starting 1 process
>>>> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>>>> error=Permission denied
>>>>
>>>> filesetup.c | 5 +----
>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/filesetup.c b/filesetup.c
>>>> index 1d3cc5ad..d77b8ba4 100644
>>>> --- a/filesetup.c
>>>> +++ b/filesetup.c
>>>> @@ -768,10 +768,7 @@ open_again:
>>>>          else
>>>>              from_hash = file_lookup_open(f, flags);
>>>>      } else if (td_read(td)) {
>>>> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>>>> -            flags |= O_RDWR;
>>>> -        else
>>>> -            flags |= O_RDONLY;
>>>> +        flags |= O_RDONLY;
>>>
>>> This will break sg like interfaces, where a read is done by writing the command to the char device.
>>>
>>> ?
>>> Jens Axboe
>>>
>>
>> Kanchan, does it work if you run fio with the --readonly option?
> 
> I think we should just make it work. Conceptually, the patch is obviously
> fine, it just happens to break some oddball cases like sg/bsg. But maybe
> we just add a fio ioengine flag for that, like FIO_RO_NEEDS_RW_OPEN or
> something, where the engine can tell us if it needs a writeable open
> even or a read-only workload.
> 
> Then generic_file_open() can use O_RDONLY for td_read(), except if the
> engine has FIO_RO_NEEDS_RW_OPEN set.
> 

Kanchan, can you try the diff below? It's also available at 
https://github.com/vincentkfu/fio/tree/rwopen

Vincent

diff --git a/engines/sg.c b/engines/sg.c
index 24783374..0bb5be4a 100644
--- a/engines/sg.c
+++ b/engines/sg.c
@@ -1428,7 +1428,7 @@ static struct ioengine_ops ioengine = {
  	.open_file	= fio_sgio_open,
  	.close_file	= fio_sgio_close,
  	.get_file_size	= fio_sgio_get_file_size,
-	.flags		= FIO_SYNCIO | FIO_RAWIO,
+	.flags		= FIO_SYNCIO | FIO_RAWIO | FIO_RO_NEEDS_RW_OPEN,
  	.options	= options,
  	.option_struct_size	= sizeof(struct sg_options)
  };
diff --git a/filesetup.c b/filesetup.c
index 1d3cc5ad..cb7047c5 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -768,7 +768,7 @@ open_again:
  		else
  			from_hash = file_lookup_open(f, flags);
  	} else if (td_read(td)) {
-		if (f->filetype == FIO_TYPE_CHAR && !read_only)
+		if (td_ioengine_flagged(td, FIO_RO_NEEDS_RW_OPEN) && !read_only)
  			flags |= O_RDWR;
  		else
  			flags |= O_RDONLY;
diff --git a/ioengines.h b/ioengines.h
index d43540d0..2cb9743e 100644
--- a/ioengines.h
+++ b/ioengines.h
@@ -89,6 +89,8 @@ enum fio_ioengine_flags {
  			= 1 << 16,	/* async ioengine with commit function that sets 
issue_time */
  	FIO_SKIPPABLE_IOMEM_ALLOC
  			= 1 << 17,	/* skip iomem_alloc & iomem_free if job sets mem/iomem */
+	FIO_RO_NEEDS_RW_OPEN
+			= 1 << 18,	/* open files in rw mode even if we have a read job */
  };

  /*
-- 

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Jens Axboe]

On 2/3/23 8:26?AM, Vincent Fu wrote:
> diff --git a/engines/sg.c b/engines/sg.c
> index 24783374..0bb5be4a 100644
> --- a/engines/sg.c
> +++ b/engines/sg.c
> @@ -1428,7 +1428,7 @@ static struct ioengine_ops ioengine = {
>      .open_file    = fio_sgio_open,
>      .close_file    = fio_sgio_close,
>      .get_file_size    = fio_sgio_get_file_size,
> -    .flags        = FIO_SYNCIO | FIO_RAWIO,
> +    .flags        = FIO_SYNCIO | FIO_RAWIO | FIO_RO_NEEDS_RW_OPEN,
>      .options    = options,
>      .option_struct_size    = sizeof(struct sg_options)
>  };
> diff --git a/filesetup.c b/filesetup.c
> index 1d3cc5ad..cb7047c5 100644
> --- a/filesetup.c
> +++ b/filesetup.c
> @@ -768,7 +768,7 @@ open_again:
>          else
>              from_hash = file_lookup_open(f, flags);
>      } else if (td_read(td)) {
> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
> +        if (td_ioengine_flagged(td, FIO_RO_NEEDS_RW_OPEN) && !read_only)
>              flags |= O_RDWR;
>          else
>              flags |= O_RDONLY;
> diff --git a/ioengines.h b/ioengines.h
> index d43540d0..2cb9743e 100644
> --- a/ioengines.h
> +++ b/ioengines.h
> @@ -89,6 +89,8 @@ enum fio_ioengine_flags {
>              = 1 << 16,    /* async ioengine with commit function that sets issue_time */
>      FIO_SKIPPABLE_IOMEM_ALLOC
>              = 1 << 17,    /* skip iomem_alloc & iomem_free if job sets mem/iomem */
> +    FIO_RO_NEEDS_RW_OPEN
> +            = 1 << 18,    /* open files in rw mode even if we have a read job */
>  };
> 
>  /*

Looks good to me.

-- 
Jens Axboe

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote:
>On 2/3/23 09:14, Jens Axboe wrote:
>>On 2/3/23 7:08?AM, Vincent Fu wrote:
>>>On 2/3/23 07:46, Jens Axboe wrote:
>>>>On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>>>>
>>>>>?Use O_RDONLY flag when read is requested on char-type files.
>>>>>
>>>>>Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>>>---
>>>>>
>>>>>Before this patch: unexpected permission-denial for unprivileged-user.
>>>>>
>>>>>$ ls -l /dev/ng0n1
>>>>>cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>>>>
>>>>>$ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>>>>>-size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>>>>>t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>>>>>4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>>>>>fio-3.33-71-g7d7a
>>>>>Starting 1 process
>>>>>fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>>>>>error=Permission denied
>>>>>
>>>>>filesetup.c | 5 +----
>>>>>1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>
>>>>>diff --git a/filesetup.c b/filesetup.c
>>>>>index 1d3cc5ad..d77b8ba4 100644
>>>>>--- a/filesetup.c
>>>>>+++ b/filesetup.c
>>>>>@@ -768,10 +768,7 @@ open_again:
>>>>>         else
>>>>>             from_hash = file_lookup_open(f, flags);
>>>>>     } else if (td_read(td)) {
>>>>>-        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>>>>>-            flags |= O_RDWR;
>>>>>-        else
>>>>>-            flags |= O_RDONLY;
>>>>>+        flags |= O_RDONLY;
>>>>
>>>>This will break sg like interfaces, where a read is done by writing the command to the char device.
>>>>
>>>>?
>>>>Jens Axboe
>>>>
>>>
>>>Kanchan, does it work if you run fio with the --readonly option?
>>
>>I think we should just make it work. Conceptually, the patch is obviously
>>fine, it just happens to break some oddball cases like sg/bsg. But maybe
>>we just add a fio ioengine flag for that, like FIO_RO_NEEDS_RW_OPEN or
>>something, where the engine can tell us if it needs a writeable open
>>even or a read-only workload.
>>
>>Then generic_file_open() can use O_RDONLY for td_read(), except if the
>>engine has FIO_RO_NEEDS_RW_OPEN set.
>>
>
>Kanchan, can you try the diff below? It's also available at https://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen

Tried, this works, thanks!
The patch looks good. Seems this flag can be added to libzbc engine too.
That also treats the the char-type open in the same way.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Jens Axboe]

On 2/5/23 11:52?PM, Kanchan Joshi wrote:
> On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote:
>> On 2/3/23 09:14, Jens Axboe wrote:
>>> On 2/3/23 7:08?AM, Vincent Fu wrote:
>>>> On 2/3/23 07:46, Jens Axboe wrote:
>>>>> On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>>>>>
>>>>>> ?Use O_RDONLY flag when read is requested on char-type files.
>>>>>>
>>>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>>>> ---
>>>>>>
>>>>>> Before this patch: unexpected permission-denial for unprivileged-user.
>>>>>>
>>>>>> $ ls -l /dev/ng0n1
>>>>>> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>>>>>
>>>>>> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>>>>>> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>>>>>> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>>>>>> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>>>>>> fio-3.33-71-g7d7a
>>>>>> Starting 1 process
>>>>>> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>>>>>> error=Permission denied
>>>>>>
>>>>>> filesetup.c | 5 +----
>>>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/filesetup.c b/filesetup.c
>>>>>> index 1d3cc5ad..d77b8ba4 100644
>>>>>> --- a/filesetup.c
>>>>>> +++ b/filesetup.c
>>>>>> @@ -768,10 +768,7 @@ open_again:
>>>>>>         else
>>>>>>             from_hash = file_lookup_open(f, flags);
>>>>>>     } else if (td_read(td)) {
>>>>>> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>>>>>> -            flags |= O_RDWR;
>>>>>> -        else
>>>>>> -            flags |= O_RDONLY;
>>>>>> +        flags |= O_RDONLY;
>>>>>
>>>>> This will break sg like interfaces, where a read is done by writing the command to the char device.
>>>>>
>>>>> ?
>>>>> Jens Axboe
>>>>>
>>>>
>>>> Kanchan, does it work if you run fio with the --readonly option?
>>>
>>> I think we should just make it work. Conceptually, the patch is obviously
>>> fine, it just happens to break some oddball cases like sg/bsg. But maybe
>>> we just add a fio ioengine flag for that, like FIO_RO_NEEDS_RW_OPEN or
>>> something, where the engine can tell us if it needs a writeable open
>>> even or a read-only workload.
>>>
>>> Then generic_file_open() can use O_RDONLY for td_read(), except if the
>>> engine has FIO_RO_NEEDS_RW_OPEN set.
>>>
>>
>> Kanchan, can you try the diff below? It's also available at https://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen
> 
> Tried, this works, thanks!
> The patch looks good. Seems this flag can be added to libzbc engine too.
> That also treats the the char-type open in the same way.

Thanks for testing - I've added it for libzbc as well.

-- 
Jens Axboe

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Vincent Fu]

On 2/6/23 14:37, Jens Axboe wrote:
> On 2/5/23 11:52?PM, Kanchan Joshi wrote:
>> On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote:
>>> On 2/3/23 09:14, Jens Axboe wrote:
>>>> On 2/3/23 7:08?AM, Vincent Fu wrote:
>>>>> On 2/3/23 07:46, Jens Axboe wrote:
>>>>>> On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>>>>>>
>>>>>>> ?Use O_RDONLY flag when read is requested on char-type files.
>>>>>>>
>>>>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Before this patch: unexpected permission-denial for unprivileged-user.
>>>>>>>
>>>>>>> $ ls -l /dev/ng0n1
>>>>>>> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>>>>>>
>>>>>>> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>>>>>>> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>>>>>>> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>>>>>>> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>>>>>>> fio-3.33-71-g7d7a
>>>>>>> Starting 1 process
>>>>>>> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>>>>>>> error=Permission denied
>>>>>>>
>>>>>>> filesetup.c | 5 +----
>>>>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/filesetup.c b/filesetup.c
>>>>>>> index 1d3cc5ad..d77b8ba4 100644
>>>>>>> --- a/filesetup.c
>>>>>>> +++ b/filesetup.c
>>>>>>> @@ -768,10 +768,7 @@ open_again:
>>>>>>>          else
>>>>>>>              from_hash = file_lookup_open(f, flags);
>>>>>>>      } else if (td_read(td)) {
>>>>>>> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>>>>>>> -            flags |= O_RDWR;
>>>>>>> -        else
>>>>>>> -            flags |= O_RDONLY;
>>>>>>> +        flags |= O_RDONLY;
>>>>>>
>>>>>> This will break sg like interfaces, where a read is done by writing the command to the char device.
>>>>>>
>>>>>> ?
>>>>>> Jens Axboe
>>>>>>
>>>>>
>>>>> Kanchan, does it work if you run fio with the --readonly option?
>>>>
>>>> I think we should just make it work. Conceptually, the patch is obviously
>>>> fine, it just happens to break some oddball cases like sg/bsg. But maybe
>>>> we just add a fio ioengine flag for that, like FIO_RO_NEEDS_RW_OPEN or
>>>> something, where the engine can tell us if it needs a writeable open
>>>> even or a read-only workload.
>>>>
>>>> Then generic_file_open() can use O_RDONLY for td_read(), except if the
>>>> engine has FIO_RO_NEEDS_RW_OPEN set.
>>>>
>>>
>>> Kanchan, can you try the diff below? It's also available at https://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen
>>
>> Tried, this works, thanks!
>> The patch looks good. Seems this flag can be added to libzbc engine too.
>> That also treats the the char-type open in the same way.
> 
> Thanks for testing - I've added it for libzbc as well.
> 

The new flag only affects generic_file_open() but the libzbc ioengine 
has its own file open function. So the flag has no effect.

Moreover libzbc uses the SG_IO ioctl even for character devices. So the 
code in libzbc_open_dev() (copied from generic_file_open) that opens 
character devices with O_RDWR is actually unnecessary.

I've committed some patches to tidy things up.

Vincent

Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Jens Axboe]

On 2/7/23 9:16?AM, Vincent Fu wrote:
> On 2/6/23 14:37, Jens Axboe wrote:
>> On 2/5/23 11:52?PM, Kanchan Joshi wrote:
>>> On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote:
>>>> On 2/3/23 09:14, Jens Axboe wrote:
>>>>> On 2/3/23 7:08?AM, Vincent Fu wrote:
>>>>>> On 2/3/23 07:46, Jens Axboe wrote:
>>>>>>> On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>>>>>>>
>>>>>>>> ?Use O_RDONLY flag when read is requested on char-type files.
>>>>>>>>
>>>>>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Before this patch: unexpected permission-denial for unprivileged-user.
>>>>>>>>
>>>>>>>> $ ls -l /dev/ng0n1
>>>>>>>> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>>>>>>>
>>>>>>>> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>>>>>>>> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>>>>>>>> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>>>>>>>> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>>>>>>>> fio-3.33-71-g7d7a
>>>>>>>> Starting 1 process
>>>>>>>> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>>>>>>>> error=Permission denied
>>>>>>>>
>>>>>>>> filesetup.c | 5 +----
>>>>>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/filesetup.c b/filesetup.c
>>>>>>>> index 1d3cc5ad..d77b8ba4 100644
>>>>>>>> --- a/filesetup.c
>>>>>>>> +++ b/filesetup.c
>>>>>>>> @@ -768,10 +768,7 @@ open_again:
>>>>>>>>          else
>>>>>>>>              from_hash = file_lookup_open(f, flags);
>>>>>>>>      } else if (td_read(td)) {
>>>>>>>> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>>>>>>>> -            flags |= O_RDWR;
>>>>>>>> -        else
>>>>>>>> -            flags |= O_RDONLY;
>>>>>>>> +        flags |= O_RDONLY;
>>>>>>>
>>>>>>> This will break sg like interfaces, where a read is done by writing the command to the char device.
>>>>>>>
>>>>>>> ?
>>>>>>> Jens Axboe
>>>>>>>
>>>>>>
>>>>>> Kanchan, does it work if you run fio with the --readonly option?
>>>>>
>>>>> I think we should just make it work. Conceptually, the patch is obviously
>>>>> fine, it just happens to break some oddball cases like sg/bsg. But maybe
>>>>> we just add a fio ioengine flag for that, like FIO_RO_NEEDS_RW_OPEN or
>>>>> something, where the engine can tell us if it needs a writeable open
>>>>> even or a read-only workload.
>>>>>
>>>>> Then generic_file_open() can use O_RDONLY for td_read(), except if the
>>>>> engine has FIO_RO_NEEDS_RW_OPEN set.
>>>>>
>>>>
>>>> Kanchan, can you try the diff below? It's also available at https://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen
>>>
>>> Tried, this works, thanks!
>>> The patch looks good. Seems this flag can be added to libzbc engine too.
>>> That also treats the the char-type open in the same way.
>>
>> Thanks for testing - I've added it for libzbc as well.
>>
> 
> The new flag only affects generic_file_open() but the libzbc ioengine
> has its own file open function. So the flag has no effect.

That's a good point, not sure how I missed that when I explicitly
checked sg for it last week...

> Moreover libzbc uses the SG_IO ioctl even for character devices. So
> the code in libzbc_open_dev() (copied from generic_file_open) that
> opens character devices with O_RDWR is actually unnecessary.
> 
> I've committed some patches to tidy things up.

Thanks!

-- 
Jens Axboe