You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Petro Karashchenko <pe...@gmail.com> on 2022/01/28 09:14:11 UTC
register_driver with 0000 mode
Hello team,
Recently I have noticed that there are many places in code where
register_driver() is called with non-zero mode with file operation
structures that have neither read nor write APIs implemented. For
example "ret = register_driver(path, &opamp_fops, 0444, dev);" while
opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
implemented. I made a PR to fix it
https://github.com/apache/incubator-nuttx/pull/5347 and change mode
from "0444" to "0000", but want to ask if anyone sees any drawback in
such an approach? Maybe I'm missing something?
Best regards,
Petro
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Yeah,
Probably this is the case. So when it is understood we need to think how to
fix it :)
Best regards,
Petro
пт, 1 квіт. 2022 р. о 17:41 Alan Carvalho de Assis <ac...@gmail.com> пише:
> Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it:
>
> #define O_RDONLY 00000000
>
> So, in fact on Linux/Mac when we are opening a file with oflag 0 we
> are opening it for reading only.
>
> On NuttX the value 0 is not defined, O_RDONLY is 1:
>
> #define O_RDONLY (1 << 0)
>
> BR,
>
> Alan
>
> On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > Hello,
> >
> > Here I'm talking not about driver registration permission, but more about
> > the "oflag" parameter to "open()" call.
> >
> > I just tried a quick example on MAC
> >
> > #include <fcntl.h>
> > #include <stdio.h>
> >
> > int main(void)
> > {
> > int fd = open("test.txt", 0);
> > if (fd < 0)
> > printf("A\n");
> > else
> > printf("B\n");
> >
> > return 0;
> > }
> >
> > The "B" is printed if the file exists. If you know the system that will
> run
> > this sample code and will print "A", please let me know.
> >
> > Best regards,
> > Petro
> >
> > пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <ac...@gmail.com>
> > пише:
> >
> >> I think the device file shouldn't be created with permission 000.
> >>
> >> Look inside your Linux /dev all device files have RW permission for
> >> root, some give only R for group and others.
> >>
> >> So, probably we need to fix the device register creation, not removing
> >> the flag check.
> >>
> >> BR,
> >>
> >> Alan
> >>
> >> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> >> > It's better to check ioctl callback too since ioctl means the driver
> >> > has
> >> > the compatibility of read(i)and write(o).
> >> >
> >> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> >> > petro.karashchenko@gmail.com> wrote:
> >> >
> >> >> So Alan do you suggest to remove inode_checkflags?
> >> >>
> >> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis
> >> >> <ac...@gmail.com>
> >> >> wrote:
> >> >>
> >> >> > Hi Petro,
> >> >> >
> >> >> > I saw your PR #1117 but I think opening a device file with flag 0
> is
> >> >> > not correct, please see the open man-pages:
> >> >> >
> >> >> > alan@dev:/tmp$ man 2 open
> >> >> >
> >> >> > The argument flags must include one of the following
> access
> >> >> > modes: O_RDONLY, O_WRONLY, or
> >> >> > O_RDWR. These request opening the file read-only,
> >> >> > write-only,
> >> >> > or read/write, respectively.
> >> >> >
> >> >> > Also the opengroup say something similar:
> >> >> >
> >> >> >
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> >> >> >
> >> >> > "Values for oflag are constructed by a bitwise-inclusive OR of
> flags
> >> >> > from the following list, defined in <fcntl.h>. Applications shall
> >> >> > specify exactly one of the first five values (file access modes)
> >> >> > below
> >> >> > in the value of oflag:"
> >> >> >
> >> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
> >> >> > according
> >> >> > to RFC2119 they are equivalents:
> >> >> >
> >> >> > https://www.ietf.org/rfc/rfc2119.txt
> >> >> >
> >> >> > BR,
> >> >> >
> >> >> > Alan
> >> >> >
> >> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com>
> wrote:
> >> >> > > Hi,
> >> >> > >
> >> >> > > I want to resume this thread again because after reexamined code
> >> >> > carefully
> >> >> > > I found that VFS layer has an API
> >> >> > >
> >> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> >> >> > > {
> >> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> >> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> >> >> > > {
> >> >> > > return -EACCES;
> >> >> > > }
> >> >> > > else
> >> >> > > {
> >> >> > > return OK;
> >> >> > > }
> >> >> > > }
> >> >> > >
> >> >> > > That checks if read and write handlers are available, so all our
> >> >> > discussion
> >> >> > > about R/W mode for IOCTL does not make any sense. We either need
> >> >> > > to
> >> >> > remove
> >> >> > > this check or register VFS nodes with proper permissions and open
> >> >> > > files
> >> >> > > with correct flags. So if the driver does not have neither read
> >> >> > > nor
> >> >> write
> >> >> > > handlers the "0000" mode should be used and "0" should be used
> >> during
> >> >> > > opening of a file. Or we need to remove "inode_checkflags()".
> >> >> > >
> >> >> > > Best regards,
> >> >> > > Petro
> >> >> > >
> >> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> >> >> > > <pe...@gmail.com>
> >> >> > > пише:
> >> >> > >
> >> >> > >> I see. Thank you for the feedback. I will rework changes to get
> >> back
> >> >> > >> read permissions.
> >> >> > >>
> >> >> > >> Best regards,
> >> >> > >> Petro
> >> >> > >>
> >> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> >> >> > >> <acassis@gmail.com
> >> >> >
> >> >> > >> пише:
> >> >> > >> >
> >> >> > >> > Hi Petro,
> >> >> > >> >
> >> >> > >> > The read permission is needed even when you just want to open
> a
> >> >> file:
> >> >> > >> >
> >> >> > >> > $ vim noreadfile
> >> >> > >> >
> >> >> > >> > $ chmod 0000 noreadfile
> >> >> > >> >
> >> >> > >> > $ ls -l noreadfile
> >> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> >> >> > >> >
> >> >> > >> > $ cat noreadfile
> >> >> > >> > cat: noreadfile: Permission denied
> >> >> > >> >
> >> >> > >> > You can even try to create a C program just to open it, and it
> >> >> > >> > will
> >> >> > >> > fail.
> >> >> > >> >
> >> >> > >> > See the man page of open function:
> >> >> > >> >
> >> >> > >> > The argument flags *must* include one of the following
> >> >> access
> >> >> > >> > modes: O_RDONLY, O_WRONLY, or
> >> >> > >> > O_RDWR. These request opening the file read-only,
> >> >> write-only,
> >> >> > >> > or read/write, respectively.
> >> >> > >> >
> >> >> > >> > This man page makes it clear you must include an access mode,
> >> >> > >> > but
> >> >> > >> > I
> >> >> > >> > passed 0 to the access mode flag of open() and it was
> accepted,
> >> >> > >> > but
> >> >> > >> > when the file has permission 0000 it returns -EPERM: "Failed
> to
> >> >> > >> > open
> >> >> > >> > file: error -1"
> >> >> > >> >
> >> >> > >> > BR,
> >> >> > >> >
> >> >> > >> > Alan
> >> >> > >> >
> >> >> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
> >> >> wrote:
> >> >> > >> > > Hello,
> >> >> > >> > >
> >> >> > >> > > Yes, but how does this relate to "0000" mode for
> >> >> > "register_driver()"?
> >> >> > >> > > Maybe you can describe some use case so it will become more
> >> >> > >> > > clear?
> >> >> > >> > > Currently ioctl works fine if driver is registered with
> >> >> > >> > > "0000"
> >> >> > >> permission
> >> >> > >> > > mode.
> >> >> > >> > >
> >> >> > >> > > Best regards,
> >> >> > >> > > Petro
> >> >> > >> > >
> >> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> >> >> > >> > > <xiaoxiang781216@gmail.com
> >> >> >
> >> >> > >> пише:
> >> >> > >> > >>
> >> >> > >> > >> If we want to do the correct permission check, the ioctl
> >> >> > >> > >> handler
> >> >> > >> needs to
> >> >> > >> > >> check R/W bit by itself based on how the ioctl is
> >> >> > >> > >> implemented.
> >> >> > >> > >> Or follow up how Linux encode the needed permission into
> >> >> > >> > >> each
> >> >> > IOCTL:
> >> >> > >> > >>
> >> >> > >>
> >> >> >
> >> >>
> >>
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> >> >> > >> > >> and let's VFS layer do the check for each driver.
> >> >> > >> > >>
> >> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> >> >> > >> > >> petro.karashchenko@gmail.com> wrote:
> >> >> > >> > >>
> >> >> > >> > >> > Hello team,
> >> >> > >> > >> >
> >> >> > >> > >> > Recently I have noticed that there are many places in
> code
> >> >> where
> >> >> > >> > >> > register_driver() is called with non-zero mode with file
> >> >> > operation
> >> >> > >> > >> > structures that have neither read nor write APIs
> >> implemented.
> >> >> For
> >> >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
> >> >> > >> > >> > dev);"
> >> >> > >> > >> > while
> >> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> >> >> "opamp_ioctl"
> >> >> > >> > >> > implemented. I made a PR to fix it
> >> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
> >> >> > >> > >> > change
> >> >> > >> > >> > mode
> >> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
> >> >> > drawback
> >> >> > >> in
> >> >> > >> > >> > such an approach? Maybe I'm missing something?
> >> >> > >> > >> >
> >> >> > >> > >> > Best regards,
> >> >> > >> > >> > Petro
> >> >> > >> > >> >
> >> >> > >> > >
> >> >> > >>
> >> >> > >
> >> >> >
> >> >>
> >> >
> >>
> >
>
Re: register_driver with 0000 mode
Posted by Gregory Nutt <sp...@gmail.com>.
> Here are a few examples of "broken" drivers:
> - opamp_fops
> - lcddev_fops
> - g_pca9635pw_fileops
> - g_foc_fops
> - notectl_fops
> - powerled_fops
> - g_rptun_devops
> - g_video_fops
>
> The list is not complete. But since the "register_driver()" does not return
> an error if both fops read and write pointers are NULL we are still in the
> middle of discussion.
>
> I just want to understand the algorithm to get it fixed. So let me
> summarise again and if nobody has any objections I will implement the
> change:
> 1. Zero "oflags" should be considered as illegal and "open" call should
> return "-EACCES" in this case.
Yes, POSIX requires this:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html "
/"... Applications shall specify exactly one of the first five
values (file access modes) below in the value of //oflag//:/
/O_EXEC/
/Open for execute only (non-directory files). The result is
unspecified if this flag is applied to a directory./
/O_RDONLY/
/Open for reading only./
/O_RDWR/
/Open for reading and writing. The result is undefined if this
flag is applied to a FIFO./
/O_SEARCH/
/Open directory for search only. The result is unspecified if
this flag is applied to a non-directory file./
/O_WRONLY/
/Open for writing only."/
(Assuming that the above are all non-zero, of course). O_EXEC and
O_SEARCH are required by the current POSIX spec but is not implemented
in NuttX.
> 2. The "register_driver()" should check if both "fops" read and write
> pointers are NULL and return an error if such a situation is detected.
I don't believe this is necessary in general. Perhaps only if
DEBUG_FEATURES is enabled? Or perhaps a DEBUG assertion?
> 3. Driver register should check if "mode" value is consistent with provided
> "fops" and if "read" method is NULL but "mode" contains "r" the error
> should be returned. The same for write and "w" permission.
An alternative, more POSIX-like implementation would flesh out the mode
logic. fs_registerdriver.s, for example, has a mode argument that is
retained in the inode. The mode could be used to determine if VFS
entity is read-able or write-able.
That would be a lot of work and most file systems do not support
permissions but would take us a long way toward a Unix-like security
system with proper permissions.
> 4. Update all the drivers that have both read and write methods as NULL and
> add "dummy_read()" or "dummy_write" handler. Optionally in order to save
> space the "fops_dummy_read" and "fops_dummy_write" can be added into
> "include/nuttx/fs/fs.h" so drivers will not need to duplicate the code and
> can reference a common implementation.
NOTE that the dummy read method returns EOF, but the dummy write returns
the write size. write never returns zero unless nbyte is zero:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
Greg
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
For what I see in Linux headers
#define O_ACCMODE 00000003
#define O_RDONLY 00000000
#define O_WRONLY 00000001
#define O_RDWR 00000002
So 3 seems to be a reserved value and because O_RDONLY in Linux is defined
to 0 the 3 value from Linux is similar to 0 value in NuttX.
Regarding "Yes, for example if I want to set the serial device baud
rate(TCSETS), should I open it with O_WRONLY?" -- I do not have a complete
answer right now due to my competence in Linux is not strong, But what I
can find in the code that _IOC_WRITE and _IOC_READ bits are not checked
against the opened file mode, but specify the direction of data transfer
during ioctl call itself if the pointer is passed as an arg so in
combination of "#define _IOC_TYPECHECK(t) (sizeof(t))" and "_IOC_SIZESHIFT"
the callee know how many bytes need to be copied. Moreover there is a
comment before _IOC_READ and _IOC_WRITE definition:
/* * Direction bits, which any architecture can choose to override *
before including this file. */
So Linux FS layer can't rely on those definitions to do a f_mode checking
and some drivers that I inspected also didn't check the R/W mode of the
file. I most probably will not have time this week to prove it, but maybe
Mr. Xiang Xiao you can find some good Linux experts at Xiaomi and clarify
this question faster than me.
My personal opinion is the following:
1. The POSIX standard clearly defines that one of the five flags should be
passed (also fopen("test.txt", ""); will return NULL) so that should be
fixed and 0 should be invalid value for oflags.
2. "register_driver()" (fs_registerdriver) interface should sanitize and
ensure that at least one of read or write is non-NULL and assert otherwise.
This should be done to ensure that "inode_checkflags()" passes.
3. "register_driver()" (fs_registerdriver) interface should sanitize and
ensure that inode "mode" parameter does not contradict with the presence of
read and write methods otherwise it should assert. In other words if "mode"
contains "r" bits set while read method is NULL or "mode" contains "w" bits
while write is NULL should lead to an assert.
4. The VFS layer should provide the "dummy" read and write handlers that
can be reused by drivers. Dummy read should always return EOF while dummy
write should return that all data have been written.
Best regards,
Petro
сб, 2 квіт. 2022 р. о 17:17 Gregory Nutt <sp...@gmail.com> пише:
>
> > Yes, for example if I want to set the serial device baud rate(TCSETS),
> > should I open it with O_WRONLY?
>
> Linux has the concept of read IOCTLs and write IOCTLs as determined by
> the IOCTL macros. My understanding is that you much provide O_RDONLY or
> O_RDWR to use the read IOCTLs and O_WRONLY or O_RDWR to use the write
> IOCTLs.
>
> But I am not sure of that and there are a lot of things I don't fully
> understand. I have seen references to some mysterious Bit 3 in the open
> flags that can be used open IOCTL-only drivers. Like
> https://github.com/NoHomey/open-ioctl:
>
> "Opens device file in non-blocking ioctl mode. File is opend with
> flags 3 | O_NONBLOCK.
>
> "Flag 3 means that only ioctl calls can be made for comunication
> with the device driver (remember read/write operations are expensive
> this is why open-ioctl was made in first place to make it easer for
> performance and command oriented device drivers)."
>
> My gut feeling is that these things are too non-standard and poorly
> thought out to be useful.
>
>
Re: register_driver with 0000 mode
Posted by Gregory Nutt <sp...@gmail.com>.
> Yes, for example if I want to set the serial device baud rate(TCSETS),
> should I open it with O_WRONLY?
Linux has the concept of read IOCTLs and write IOCTLs as determined by
the IOCTL macros. My understanding is that you much provide O_RDONLY or
O_RDWR to use the read IOCTLs and O_WRONLY or O_RDWR to use the write
IOCTLs.
But I am not sure of that and there are a lot of things I don't fully
understand. I have seen references to some mysterious Bit 3 in the open
flags that can be used open IOCTL-only drivers. Like
https://github.com/NoHomey/open-ioctl:
"Opens device file in non-blocking ioctl mode. File is opend with
flags 3 | O_NONBLOCK.
"Flag 3 means that only ioctl calls can be made for comunication
with the device driver (remember read/write operations are expensive
this is why open-ioctl was made in first place to make it easer for
performance and command oriented device drivers)."
My gut feeling is that these things are too non-standard and poorly
thought out to be useful.
Re: register_driver with 0000 mode
Posted by Xiang Xiao <xi...@gmail.com>.
Yes, for example if I want to set the serial device baud rate(TCSETS),
should I open it with O_WRONLY?
On Sat, Apr 2, 2022 at 10:56 PM Gregory Nutt <sp...@gmail.com> wrote:
> On 4/2/2022 7:03 AM, Xiang Xiao wrote:
> > Many functionality is accessed through ioctl callback, what permission is
> > needed before invoking ioctl?
>
> By permissions, you mean open mode? Per POSIX, O_WRONLY, O_RDONLY, or
> O_RDWR is required on any success open call (or O_EXEC or O_SEARCH).
>
> But I did run across this non-standard Linux behavior in this man page:
> https://man7.org/linux/man-pages/man2/open.2.html:
>
> Under Linux, the*O_NONBLOCK *flag is sometimes used in cases where
> one wants to open but does not necessarily have the intention to
> read or write. For example, this may be used to open a device in
> order to get a file descriptor for use withioctl(2) <
> https://man7.org/linux/man-pages/man2/ioctl.2.html>.
>
>
>
Re: register_driver with 0000 mode
Posted by Gregory Nutt <sp...@gmail.com>.
On 4/2/2022 7:03 AM, Xiang Xiao wrote:
> Many functionality is accessed through ioctl callback, what permission is
> needed before invoking ioctl?
By permissions, you mean open mode? Per POSIX, O_WRONLY, O_RDONLY, or
O_RDWR is required on any success open call (or O_EXEC or O_SEARCH).
But I did run across this non-standard Linux behavior in this man page:
https://man7.org/linux/man-pages/man2/open.2.html:
Under Linux, the*O_NONBLOCK *flag is sometimes used in cases where
one wants to open but does not necessarily have the intention to
read or write. For example, this may be used to open a device in
order to get a file descriptor for use withioctl(2) <https://man7.org/linux/man-pages/man2/ioctl.2.html>.
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Hi,
Here are a few examples of "broken" drivers:
- opamp_fops
- lcddev_fops
- g_pca9635pw_fileops
- g_foc_fops
- notectl_fops
- powerled_fops
- g_rptun_devops
- g_video_fops
The list is not complete. But since the "register_driver()" does not return
an error if both fops read and write pointers are NULL we are still in the
middle of discussion.
I just want to understand the algorithm to get it fixed. So let me
summarise again and if nobody has any objections I will implement the
change:
1. Zero "oflags" should be considered as illegal and "open" call should
return "-EACCES" in this case.
2. The "register_driver()" should check if both "fops" read and write
pointers are NULL and return an error if such a situation is detected.
3. Driver register should check if "mode" value is consistent with provided
"fops" and if "read" method is NULL but "mode" contains "r" the error
should be returned. The same for write and "w" permission.
4. Update all the drivers that have both read and write methods as NULL and
add "dummy_read()" or "dummy_write" handler. Optionally in order to save
space the "fops_dummy_read" and "fops_dummy_write" can be added into
"include/nuttx/fs/fs.h" so drivers will not need to duplicate the code and
can reference a common implementation.
Please reply with your thoughts.
Best regards,
Petro
сб, 2 квіт. 2022 р. о 15:08 Gregory Nutt <sp...@gmail.com> пише:
> > By doing so we can relax check in inode_checkflags() to:
> >
> > int inode_checkflags(FAR struct inode *inode, int oflags)
> > {
> > if ((oflags == 0) ||
> > (oflags & O_WROK) != 0 && !inode->u.i_ops->write)
> > {
> > return -EACCES;
> > }
> > else
> > {
> > return OK;
> > }
> > }
> >
> > No, this would be an error. This would break any file, filesystem, or
> driver that really needs to support write-only behavior. In that case,
> there is a write method and no read method.
>
> This would be a major loss of functionality. These IOCTL-only drivers are
> degenerate cases and perhaps for them you can cut corners. Bu, we need to
> support all existing behavior for all VFS entities.
>
> To permit opening with O_RDWR, some IOCTL drivers have dummy write methods
> too. The loop driver again has:
>
>
> /****************************************************************************
> * Name: loop_write
>
> ****************************************************************************/
> static ssize_t loop_write(FAR struct file *filep, FAR const char *buffer,
> size_t len)
> {
> return len; /* Say that everything was written */
> }
>
Re: register_driver with 0000 mode
Posted by Gregory Nutt <sp...@gmail.com>.
> By doing so we can relax check in inode_checkflags() to:
>
> int inode_checkflags(FAR struct inode *inode, int oflags)
> {
> if ((oflags == 0) ||
> (oflags & O_WROK) != 0 && !inode->u.i_ops->write)
> {
> return -EACCES;
> }
> else
> {
> return OK;
> }
> }
>
> No, this would be an error. This would break any file, filesystem, or
driver that really needs to support write-only behavior. In that case,
there is a write method and no read method.
This would be a major loss of functionality. These IOCTL-only drivers are
degenerate cases and perhaps for them you can cut corners. Bu, we need to
support all existing behavior for all VFS entities.
To permit opening with O_RDWR, some IOCTL drivers have dummy write methods
too. The loop driver again has:
/****************************************************************************
* Name: loop_write
****************************************************************************/
static ssize_t loop_write(FAR struct file *filep, FAR const char *buffer,
size_t len)
{
return len; /* Say that everything was written */
}
Re: register_driver with 0000 mode
Posted by Xiang Xiao <xi...@gmail.com>.
Many functionality is accessed through ioctl callback, what permission is
needed before invoking ioctl?
On Sat, Apr 2, 2022 at 8:45 PM Gregory Nutt <sp...@gmail.com> wrote:
> If I understand correctly we can add one single "dummy_read" implementation
> > for all drivers that do not supply the read method. This will ensure that
> > O_RDOK is always supported if file permissions allow it.
> >
>
> There aren't any file permissions. The entire access is controlled by NULL
> and non-NULL read/write methods.
>
> That is the way that it has worked. if you check, I think you will find
> that all of the existing IOCTL drivers already have dummy read and
> sometimes write methods. Being consistent is important to the integrity of
> the OS. Whatever you all decide to do, make sure that is is consistent
> across all drivers.
>
> The semantics are: NULL read -> write only, NULL write -> read only. Both
> NULL -> the driver is broken.
>
> I don't understand how checking if the pointer is NULL would support
> read-only, or write-only behavior. I think we would be losing
> functionality and this is important in other drivers where read and write
> actually matter.
>
Re: register_driver with 0000 mode
Posted by Gregory Nutt <sp...@gmail.com>.
If I understand correctly we can add one single "dummy_read" implementation
> for all drivers that do not supply the read method. This will ensure that
> O_RDOK is always supported if file permissions allow it.
>
There aren't any file permissions. The entire access is controlled by NULL
and non-NULL read/write methods.
That is the way that it has worked. if you check, I think you will find
that all of the existing IOCTL drivers already have dummy read and
sometimes write methods. Being consistent is important to the integrity of
the OS. Whatever you all decide to do, make sure that is is consistent
across all drivers.
The semantics are: NULL read -> write only, NULL write -> read only. Both
NULL -> the driver is broken.
I don't understand how checking if the pointer is NULL would support
read-only, or write-only behavior. I think we would be losing
functionality and this is important in other drivers where read and write
actually matter.
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Hi,
If I understand correctly we can add one single "dummy_read" implementation
for all drivers that do not supply the read method. This will ensure that
O_RDOK is always supported if file permissions allow it. By doing so we can
relax check in inode_checkflags() to:
int inode_checkflags(FAR struct inode *inode, int oflags)
{
if ((oflags == 0) ||
(oflags & O_WROK) != 0 && !inode->u.i_ops->write)
{
return -EACCES;
}
else
{
return OK;
}
}
Also the "0" oflags will not be allowed.
If yes, then I'm happy to submit the change. Otherwise please correct me if
I'm wrong.
Best regards,
Petro
сб, 2 квіт. 2022 р. о 08:06 Jukka Laitinen <ju...@iki.fi> пише:
> As Greg said, this is a traditional way. I'd maybe allow dummy
> reads/writes if permissions allow, but keep that logic in vfs by checking
> if the pointer is initialized. Then you don't need to provide the dummy
> implementation in every driver.
>
> Petro Karashchenko kirjoitti lauantai 2. huhtikuuta 2022:
> > Hello Gregory,
> >
> > Do you recommend not to allow registration of a driver that does not have
> > read method and make a requirement for the driver to provide at least
> dummy
> > read?
> >
> > Best regards,
> > Petro
> >
> > On Fri, Apr 1, 2022, 9:48 PM Gregory Nutt <sp...@gmail.com> wrote:
> >
> > >
> > > > One option, conforming standard, would be that you just always give
> > > O_RDWR (same flags as what linux devices have), but then when calling
> > > read/write you check if the pointer is non-null. If the driver doesn't
> > > define read or write, those operations are allowed on the device, but
> act
> > > as no-op.
> > >
> > > If you can't think of anything useful to do with read() or write(),
> > > thenthis has been historically handled is by including a dummy read
> > > method in the driver that just returns zero (EOF). For example, the
> > > loop driver:
> > >
> > >
> > >
> /****************************************************************************
> > > * Name: loop_read
> > >
> > >
> ****************************************************************************/
> > >
> > > static ssize_t loop_read(FAR struct file *filep, FAR char *buffer,
> > > size_t len)
> > > {
> > > return 0; /* Return EOF */
> > > }
> > > *
> > > *
> > >
> >
Re: register_driver with 0000 mode
Posted by Jukka Laitinen <ju...@iki.fi>.
As Greg said, this is a traditional way. I'd maybe allow dummy reads/writes if permissions allow, but keep that logic in vfs by checking if the pointer is initialized. Then you don't need to provide the dummy implementation in every driver.
Petro Karashchenko kirjoitti lauantai 2. huhtikuuta 2022:
> Hello Gregory,
>
> Do you recommend not to allow registration of a driver that does not have
> read method and make a requirement for the driver to provide at least dummy
> read?
>
> Best regards,
> Petro
>
> On Fri, Apr 1, 2022, 9:48 PM Gregory Nutt <sp...@gmail.com> wrote:
>
> >
> > > One option, conforming standard, would be that you just always give
> > O_RDWR (same flags as what linux devices have), but then when calling
> > read/write you check if the pointer is non-null. If the driver doesn't
> > define read or write, those operations are allowed on the device, but act
> > as no-op.
> >
> > If you can't think of anything useful to do with read() or write(),
> > thenthis has been historically handled is by including a dummy read
> > method in the driver that just returns zero (EOF). For example, the
> > loop driver:
> >
> >
> > /****************************************************************************
> > * Name: loop_read
> >
> > ****************************************************************************/
> >
> > static ssize_t loop_read(FAR struct file *filep, FAR char *buffer,
> > size_t len)
> > {
> > return 0; /* Return EOF */
> > }
> > *
> > *
> >
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Hello Gregory,
Do you recommend not to allow registration of a driver that does not have
read method and make a requirement for the driver to provide at least dummy
read?
Best regards,
Petro
On Fri, Apr 1, 2022, 9:48 PM Gregory Nutt <sp...@gmail.com> wrote:
>
> > One option, conforming standard, would be that you just always give
> O_RDWR (same flags as what linux devices have), but then when calling
> read/write you check if the pointer is non-null. If the driver doesn't
> define read or write, those operations are allowed on the device, but act
> as no-op.
>
> If you can't think of anything useful to do with read() or write(),
> thenthis has been historically handled is by including a dummy read
> method in the driver that just returns zero (EOF). For example, the
> loop driver:
>
>
> /****************************************************************************
> * Name: loop_read
>
> ****************************************************************************/
>
> static ssize_t loop_read(FAR struct file *filep, FAR char *buffer,
> size_t len)
> {
> return 0; /* Return EOF */
> }
> *
> *
>
Re: register_driver with 0000 mode
Posted by Gregory Nutt <sp...@gmail.com>.
> One option, conforming standard, would be that you just always give O_RDWR (same flags as what linux devices have), but then when calling read/write you check if the pointer is non-null. If the driver doesn't define read or write, those operations are allowed on the device, but act as no-op.
If you can't think of anything useful to do with read() or write(),
thenthis has been historically handled is by including a dummy read
method in the driver that just returns zero (EOF). For example, the
loop driver:
/****************************************************************************
* Name: loop_read
****************************************************************************/
static ssize_t loop_read(FAR struct file *filep, FAR char *buffer,
size_t len)
{
return 0; /* Return EOF */
}
*
*
Re: register_driver with 0000 mode
Posted by Jukka Laitinen <ju...@iki.fi>.
One option, conforming standard, would be that you just always give O_RDWR (same flags as what linux devices have), but then when calling read/write you check if the pointer is non-null. If the driver doesn't define read or write, those operations are allowed on the device, but act as no-op.
- Jukka
Jukka Laitinen kirjoitti perjantai 1. huhtikuuta 2022:
> In my opinion 0, if you are asking that, but it is strictly not conforming the standard.
>
> Posix says that "Applications shall specify exactly one of the first five...", so there is no correct standard conforming way. All five would be wrong, imho.
>
> -Jukka
>
> Petro Karashchenko kirjoitti perjantai 1. huhtikuuta 2022:
> > Yes Jukka what you are saying is absolutely correct. The main item under
> > discussion are the drivers that are pure ioctl (that means do not have
> > neither read nor write handlers). Should RD or WR flag be passed to open
> > call in such case of or 0 should be passed.
> >
> > Best regards,
> > Petro
> >
> > On Fri, Apr 1, 2022, 6:54 PM Jukka Laitinen <ju...@iki.fi> wrote:
> >
> > > Different posix implementations have different values for these flags, so
> > > I think it is ok not to have the same as what linux has.
> > >
> > > Posix (2017) specifies thar exactly one of the following is provided for
> > > open: O_EXEC, O_RDWR, O_RDONLY, O_SEARCH and O_WRONLY, and other flags are
> > > bitwise OR'd to that. The spec says nothing about that you can just give
> > > "0" afaik, it just happens to be that in Linux O_RDONLY happens to be 0.
> > >
> > > Maybe just fix the open having O_RDONLY in places where you really open
> > > the file as read-only, O_WRONLY for write only and O? That should be
> > > according to the standard at least.
> > >
> > > Just my 2 cents
> > >
> > > - Jukka
> > >
> > >
> > >
> > > Alan Carvalho de Assis kirjoitti perjantai 1. huhtikuuta 2022:
> > > > More about it here:
> > > >
> > > https://stackoverflow.com/questions/61923703/how-to-make-sense-of-o-rdonly-0
> > > >
> > > > So, I agree with the comment that said "calling it flag is misnomer
> > > > and misleading"
> > > >
> > > > On Unix/Linux O_RDONLY | O_WRONLY != O_RDWR, on NuttX is it explicitly
> > > this way:
> > > >
> > > > #define O_RDWR (O_RDOK|O_WROK) /* Open for both read & write access
> > > */
> > > >
> > > > Now, I'm also confuse about the right thing to do:
> > > >
> > > > 1) Fix include/fcntl.h to follow Unix/Linux
> > > > 2) Keep things we they are and don't accept opening a file if it
> > > > doesn't have RDONLY flag (and change all the drivers, event ioctl only
> > > > drivers, to include reading flag)
> > > > 3) Remove the flag checking.
> > > >
> > > > Probably we should do 1) because NuttX follows Unix/Linux approach,
> > > > although I agree that Unix/Linux are completely non-sense on this
> > > > subject, oflag should be a flag like it is on NuttX :-)
> > > >
> > > > BR,
> > > >
> > > > Alan
> > > >
> > > > On 4/1/22, Alan Carvalho de Assis <ac...@gmail.com> wrote:
> > > > > Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it:
> > > > >
> > > > > #define O_RDONLY 00000000
> > > > >
> > > > > So, in fact on Linux/Mac when we are opening a file with oflag 0 we
> > > > > are opening it for reading only.
> > > > >
> > > > > On NuttX the value 0 is not defined, O_RDONLY is 1:
> > > > >
> > > > > #define O_RDONLY (1 << 0)
> > > > >
> > > > > BR,
> > > > >
> > > > > Alan
> > > > >
> > > > > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > > > >> Hello,
> > > > >>
> > > > >> Here I'm talking not about driver registration permission, but more
> > > about
> > > > >> the "oflag" parameter to "open()" call.
> > > > >>
> > > > >> I just tried a quick example on MAC
> > > > >>
> > > > >> #include <fcntl.h>
> > > > >> #include <stdio.h>
> > > > >>
> > > > >> int main(void)
> > > > >> {
> > > > >> int fd = open("test.txt", 0);
> > > > >> if (fd < 0)
> > > > >> printf("A\n");
> > > > >> else
> > > > >> printf("B\n");
> > > > >>
> > > > >> return 0;
> > > > >> }
> > > > >>
> > > > >> The "B" is printed if the file exists. If you know the system that
> > > will
> > > > >> run
> > > > >> this sample code and will print "A", please let me know.
> > > > >>
> > > > >> Best regards,
> > > > >> Petro
> > > > >>
> > > > >> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <acassis@gmail.com
> > > >
> > > > >> пише:
> > > > >>
> > > > >>> I think the device file shouldn't be created with permission 000.
> > > > >>>
> > > > >>> Look inside your Linux /dev all device files have RW permission for
> > > > >>> root, some give only R for group and others.
> > > > >>>
> > > > >>> So, probably we need to fix the device register creation, not
> > > removing
> > > > >>> the flag check.
> > > > >>>
> > > > >>> BR,
> > > > >>>
> > > > >>> Alan
> > > > >>>
> > > > >>> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> > > > >>> > It's better to check ioctl callback too since ioctl means the
> > > driver
> > > > >>> > has
> > > > >>> > the compatibility of read(i)and write(o).
> > > > >>> >
> > > > >>> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> > > > >>> > petro.karashchenko@gmail.com> wrote:
> > > > >>> >
> > > > >>> >> So Alan do you suggest to remove inode_checkflags?
> > > > >>> >>
> > > > >>> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis
> > > > >>> >> <ac...@gmail.com>
> > > > >>> >> wrote:
> > > > >>> >>
> > > > >>> >> > Hi Petro,
> > > > >>> >> >
> > > > >>> >> > I saw your PR #1117 but I think opening a device file with flag
> > > 0
> > > > >>> >> > is
> > > > >>> >> > not correct, please see the open man-pages:
> > > > >>> >> >
> > > > >>> >> > alan@dev:/tmp$ man 2 open
> > > > >>> >> >
> > > > >>> >> > The argument flags must include one of the following
> > > > >>> >> > access
> > > > >>> >> > modes: O_RDONLY, O_WRONLY, or
> > > > >>> >> > O_RDWR. These request opening the file read-only,
> > > > >>> >> > write-only,
> > > > >>> >> > or read/write, respectively.
> > > > >>> >> >
> > > > >>> >> > Also the opengroup say something similar:
> > > > >>> >> >
> > > > >>> >> >
> > > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> > > > >>> >> >
> > > > >>> >> > "Values for oflag are constructed by a bitwise-inclusive OR of
> > > > >>> >> > flags
> > > > >>> >> > from the following list, defined in <fcntl.h>. Applications
> > > shall
> > > > >>> >> > specify exactly one of the first five values (file access modes)
> > > > >>> >> > below
> > > > >>> >> > in the value of oflag:"
> > > > >>> >> >
> > > > >>> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
> > > > >>> >> > according
> > > > >>> >> > to RFC2119 they are equivalents:
> > > > >>> >> >
> > > > >>> >> > https://www.ietf.org/rfc/rfc2119.txt
> > > > >>> >> >
> > > > >>> >> > BR,
> > > > >>> >> >
> > > > >>> >> > Alan
> > > > >>> >> >
> > > > >>> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com>
> > > wrote:
> > > > >>> >> > > Hi,
> > > > >>> >> > >
> > > > >>> >> > > I want to resume this thread again because after reexamined
> > > code
> > > > >>> >> > carefully
> > > > >>> >> > > I found that VFS layer has an API
> > > > >>> >> > >
> > > > >>> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> > > > >>> >> > > {
> > > > >>> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> > > > >>> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> > > > >>> >> > > {
> > > > >>> >> > > return -EACCES;
> > > > >>> >> > > }
> > > > >>> >> > > else
> > > > >>> >> > > {
> > > > >>> >> > > return OK;
> > > > >>> >> > > }
> > > > >>> >> > > }
> > > > >>> >> > >
> > > > >>> >> > > That checks if read and write handlers are available, so all
> > > our
> > > > >>> >> > discussion
> > > > >>> >> > > about R/W mode for IOCTL does not make any sense. We either
> > > need
> > > > >>> >> > > to
> > > > >>> >> > remove
> > > > >>> >> > > this check or register VFS nodes with proper permissions and
> > > open
> > > > >>> >> > > files
> > > > >>> >> > > with correct flags. So if the driver does not have neither
> > > read
> > > > >>> >> > > nor
> > > > >>> >> write
> > > > >>> >> > > handlers the "0000" mode should be used and "0" should be used
> > > > >>> during
> > > > >>> >> > > opening of a file. Or we need to remove "inode_checkflags()".
> > > > >>> >> > >
> > > > >>> >> > > Best regards,
> > > > >>> >> > > Petro
> > > > >>> >> > >
> > > > >>> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> > > > >>> >> > > <pe...@gmail.com>
> > > > >>> >> > > пише:
> > > > >>> >> > >
> > > > >>> >> > >> I see. Thank you for the feedback. I will rework changes to
> > > get
> > > > >>> back
> > > > >>> >> > >> read permissions.
> > > > >>> >> > >>
> > > > >>> >> > >> Best regards,
> > > > >>> >> > >> Petro
> > > > >>> >> > >>
> > > > >>> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> > > > >>> >> > >> <acassis@gmail.com
> > > > >>> >> >
> > > > >>> >> > >> пише:
> > > > >>> >> > >> >
> > > > >>> >> > >> > Hi Petro,
> > > > >>> >> > >> >
> > > > >>> >> > >> > The read permission is needed even when you just want to
> > > open
> > > > >>> >> > >> > a
> > > > >>> >> file:
> > > > >>> >> > >> >
> > > > >>> >> > >> > $ vim noreadfile
> > > > >>> >> > >> >
> > > > >>> >> > >> > $ chmod 0000 noreadfile
> > > > >>> >> > >> >
> > > > >>> >> > >> > $ ls -l noreadfile
> > > > >>> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> > > > >>> >> > >> >
> > > > >>> >> > >> > $ cat noreadfile
> > > > >>> >> > >> > cat: noreadfile: Permission denied
> > > > >>> >> > >> >
> > > > >>> >> > >> > You can even try to create a C program just to open it,
> > > and it
> > > > >>> >> > >> > will
> > > > >>> >> > >> > fail.
> > > > >>> >> > >> >
> > > > >>> >> > >> > See the man page of open function:
> > > > >>> >> > >> >
> > > > >>> >> > >> > The argument flags *must* include one of the
> > > following
> > > > >>> >> access
> > > > >>> >> > >> > modes: O_RDONLY, O_WRONLY, or
> > > > >>> >> > >> > O_RDWR. These request opening the file read-only,
> > > > >>> >> write-only,
> > > > >>> >> > >> > or read/write, respectively.
> > > > >>> >> > >> >
> > > > >>> >> > >> > This man page makes it clear you must include an access
> > > mode,
> > > > >>> >> > >> > but
> > > > >>> >> > >> > I
> > > > >>> >> > >> > passed 0 to the access mode flag of open() and it was
> > > > >>> >> > >> > accepted,
> > > > >>> >> > >> > but
> > > > >>> >> > >> > when the file has permission 0000 it returns -EPERM:
> > > "Failed
> > > > >>> >> > >> > to
> > > > >>> >> > >> > open
> > > > >>> >> > >> > file: error -1"
> > > > >>> >> > >> >
> > > > >>> >> > >> > BR,
> > > > >>> >> > >> >
> > > > >>> >> > >> > Alan
> > > > >>> >> > >> >
> > > > >>> >> > >> > On 1/28/22, Petro Karashchenko <
> > > petro.karashchenko@gmail.com>
> > > > >>> >> wrote:
> > > > >>> >> > >> > > Hello,
> > > > >>> >> > >> > >
> > > > >>> >> > >> > > Yes, but how does this relate to "0000" mode for
> > > > >>> >> > "register_driver()"?
> > > > >>> >> > >> > > Maybe you can describe some use case so it will become
> > > more
> > > > >>> >> > >> > > clear?
> > > > >>> >> > >> > > Currently ioctl works fine if driver is registered with
> > > > >>> >> > >> > > "0000"
> > > > >>> >> > >> permission
> > > > >>> >> > >> > > mode.
> > > > >>> >> > >> > >
> > > > >>> >> > >> > > Best regards,
> > > > >>> >> > >> > > Petro
> > > > >>> >> > >> > >
> > > > >>> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> > > > >>> >> > >> > > <xiaoxiang781216@gmail.com
> > > > >>> >> >
> > > > >>> >> > >> пише:
> > > > >>> >> > >> > >>
> > > > >>> >> > >> > >> If we want to do the correct permission check, the ioctl
> > > > >>> >> > >> > >> handler
> > > > >>> >> > >> needs to
> > > > >>> >> > >> > >> check R/W bit by itself based on how the ioctl is
> > > > >>> >> > >> > >> implemented.
> > > > >>> >> > >> > >> Or follow up how Linux encode the needed permission into
> > > > >>> >> > >> > >> each
> > > > >>> >> > IOCTL:
> > > > >>> >> > >> > >>
> > > > >>> >> > >>
> > > > >>> >> >
> > > > >>> >>
> > > > >>>
> > > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > > > >>> >> > >> > >> and let's VFS layer do the check for each driver.
> > > > >>> >> > >> > >>
> > > > >>> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > > > >>> >> > >> > >> petro.karashchenko@gmail.com> wrote:
> > > > >>> >> > >> > >>
> > > > >>> >> > >> > >> > Hello team,
> > > > >>> >> > >> > >> >
> > > > >>> >> > >> > >> > Recently I have noticed that there are many places in
> > > > >>> >> > >> > >> > code
> > > > >>> >> where
> > > > >>> >> > >> > >> > register_driver() is called with non-zero mode with
> > > file
> > > > >>> >> > operation
> > > > >>> >> > >> > >> > structures that have neither read nor write APIs
> > > > >>> implemented.
> > > > >>> >> For
> > > > >>> >> > >> > >> > example "ret = register_driver(path, &opamp_fops,
> > > 0444,
> > > > >>> >> > >> > >> > dev);"
> > > > >>> >> > >> > >> > while
> > > > >>> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> > > > >>> >> "opamp_ioctl"
> > > > >>> >> > >> > >> > implemented. I made a PR to fix it
> > > > >>> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347
> > > and
> > > > >>> >> > >> > >> > change
> > > > >>> >> > >> > >> > mode
> > > > >>> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees
> > > any
> > > > >>> >> > drawback
> > > > >>> >> > >> in
> > > > >>> >> > >> > >> > such an approach? Maybe I'm missing something?
> > > > >>> >> > >> > >> >
> > > > >>> >> > >> > >> > Best regards,
> > > > >>> >> > >> > >> > Petro
> > > > >>> >> > >> > >> >
> > > > >>> >> > >> > >
> > > > >>> >> > >>
> > > > >>> >> > >
> > > > >>> >> >
> > > > >>> >>
> > > > >>> >
> > > > >>>
> > > > >>
> > > > >
> > > >
>
Re: register_driver with 0000 mode
Posted by Jukka Laitinen <ju...@iki.fi>.
In my opinion 0, if you are asking that, but it is strictly not conforming the standard.
Posix says that "Applications shall specify exactly one of the first five...", so there is no correct standard conforming way. All five would be wrong, imho.
-Jukka
Petro Karashchenko kirjoitti perjantai 1. huhtikuuta 2022:
> Yes Jukka what you are saying is absolutely correct. The main item under
> discussion are the drivers that are pure ioctl (that means do not have
> neither read nor write handlers). Should RD or WR flag be passed to open
> call in such case of or 0 should be passed.
>
> Best regards,
> Petro
>
> On Fri, Apr 1, 2022, 6:54 PM Jukka Laitinen <ju...@iki.fi> wrote:
>
> > Different posix implementations have different values for these flags, so
> > I think it is ok not to have the same as what linux has.
> >
> > Posix (2017) specifies thar exactly one of the following is provided for
> > open: O_EXEC, O_RDWR, O_RDONLY, O_SEARCH and O_WRONLY, and other flags are
> > bitwise OR'd to that. The spec says nothing about that you can just give
> > "0" afaik, it just happens to be that in Linux O_RDONLY happens to be 0.
> >
> > Maybe just fix the open having O_RDONLY in places where you really open
> > the file as read-only, O_WRONLY for write only and O? That should be
> > according to the standard at least.
> >
> > Just my 2 cents
> >
> > - Jukka
> >
> >
> >
> > Alan Carvalho de Assis kirjoitti perjantai 1. huhtikuuta 2022:
> > > More about it here:
> > >
> > https://stackoverflow.com/questions/61923703/how-to-make-sense-of-o-rdonly-0
> > >
> > > So, I agree with the comment that said "calling it flag is misnomer
> > > and misleading"
> > >
> > > On Unix/Linux O_RDONLY | O_WRONLY != O_RDWR, on NuttX is it explicitly
> > this way:
> > >
> > > #define O_RDWR (O_RDOK|O_WROK) /* Open for both read & write access
> > */
> > >
> > > Now, I'm also confuse about the right thing to do:
> > >
> > > 1) Fix include/fcntl.h to follow Unix/Linux
> > > 2) Keep things we they are and don't accept opening a file if it
> > > doesn't have RDONLY flag (and change all the drivers, event ioctl only
> > > drivers, to include reading flag)
> > > 3) Remove the flag checking.
> > >
> > > Probably we should do 1) because NuttX follows Unix/Linux approach,
> > > although I agree that Unix/Linux are completely non-sense on this
> > > subject, oflag should be a flag like it is on NuttX :-)
> > >
> > > BR,
> > >
> > > Alan
> > >
> > > On 4/1/22, Alan Carvalho de Assis <ac...@gmail.com> wrote:
> > > > Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it:
> > > >
> > > > #define O_RDONLY 00000000
> > > >
> > > > So, in fact on Linux/Mac when we are opening a file with oflag 0 we
> > > > are opening it for reading only.
> > > >
> > > > On NuttX the value 0 is not defined, O_RDONLY is 1:
> > > >
> > > > #define O_RDONLY (1 << 0)
> > > >
> > > > BR,
> > > >
> > > > Alan
> > > >
> > > > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > > >> Hello,
> > > >>
> > > >> Here I'm talking not about driver registration permission, but more
> > about
> > > >> the "oflag" parameter to "open()" call.
> > > >>
> > > >> I just tried a quick example on MAC
> > > >>
> > > >> #include <fcntl.h>
> > > >> #include <stdio.h>
> > > >>
> > > >> int main(void)
> > > >> {
> > > >> int fd = open("test.txt", 0);
> > > >> if (fd < 0)
> > > >> printf("A\n");
> > > >> else
> > > >> printf("B\n");
> > > >>
> > > >> return 0;
> > > >> }
> > > >>
> > > >> The "B" is printed if the file exists. If you know the system that
> > will
> > > >> run
> > > >> this sample code and will print "A", please let me know.
> > > >>
> > > >> Best regards,
> > > >> Petro
> > > >>
> > > >> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <acassis@gmail.com
> > >
> > > >> пише:
> > > >>
> > > >>> I think the device file shouldn't be created with permission 000.
> > > >>>
> > > >>> Look inside your Linux /dev all device files have RW permission for
> > > >>> root, some give only R for group and others.
> > > >>>
> > > >>> So, probably we need to fix the device register creation, not
> > removing
> > > >>> the flag check.
> > > >>>
> > > >>> BR,
> > > >>>
> > > >>> Alan
> > > >>>
> > > >>> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> > > >>> > It's better to check ioctl callback too since ioctl means the
> > driver
> > > >>> > has
> > > >>> > the compatibility of read(i)and write(o).
> > > >>> >
> > > >>> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> > > >>> > petro.karashchenko@gmail.com> wrote:
> > > >>> >
> > > >>> >> So Alan do you suggest to remove inode_checkflags?
> > > >>> >>
> > > >>> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis
> > > >>> >> <ac...@gmail.com>
> > > >>> >> wrote:
> > > >>> >>
> > > >>> >> > Hi Petro,
> > > >>> >> >
> > > >>> >> > I saw your PR #1117 but I think opening a device file with flag
> > 0
> > > >>> >> > is
> > > >>> >> > not correct, please see the open man-pages:
> > > >>> >> >
> > > >>> >> > alan@dev:/tmp$ man 2 open
> > > >>> >> >
> > > >>> >> > The argument flags must include one of the following
> > > >>> >> > access
> > > >>> >> > modes: O_RDONLY, O_WRONLY, or
> > > >>> >> > O_RDWR. These request opening the file read-only,
> > > >>> >> > write-only,
> > > >>> >> > or read/write, respectively.
> > > >>> >> >
> > > >>> >> > Also the opengroup say something similar:
> > > >>> >> >
> > > >>> >> >
> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> > > >>> >> >
> > > >>> >> > "Values for oflag are constructed by a bitwise-inclusive OR of
> > > >>> >> > flags
> > > >>> >> > from the following list, defined in <fcntl.h>. Applications
> > shall
> > > >>> >> > specify exactly one of the first five values (file access modes)
> > > >>> >> > below
> > > >>> >> > in the value of oflag:"
> > > >>> >> >
> > > >>> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
> > > >>> >> > according
> > > >>> >> > to RFC2119 they are equivalents:
> > > >>> >> >
> > > >>> >> > https://www.ietf.org/rfc/rfc2119.txt
> > > >>> >> >
> > > >>> >> > BR,
> > > >>> >> >
> > > >>> >> > Alan
> > > >>> >> >
> > > >>> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com>
> > wrote:
> > > >>> >> > > Hi,
> > > >>> >> > >
> > > >>> >> > > I want to resume this thread again because after reexamined
> > code
> > > >>> >> > carefully
> > > >>> >> > > I found that VFS layer has an API
> > > >>> >> > >
> > > >>> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> > > >>> >> > > {
> > > >>> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> > > >>> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> > > >>> >> > > {
> > > >>> >> > > return -EACCES;
> > > >>> >> > > }
> > > >>> >> > > else
> > > >>> >> > > {
> > > >>> >> > > return OK;
> > > >>> >> > > }
> > > >>> >> > > }
> > > >>> >> > >
> > > >>> >> > > That checks if read and write handlers are available, so all
> > our
> > > >>> >> > discussion
> > > >>> >> > > about R/W mode for IOCTL does not make any sense. We either
> > need
> > > >>> >> > > to
> > > >>> >> > remove
> > > >>> >> > > this check or register VFS nodes with proper permissions and
> > open
> > > >>> >> > > files
> > > >>> >> > > with correct flags. So if the driver does not have neither
> > read
> > > >>> >> > > nor
> > > >>> >> write
> > > >>> >> > > handlers the "0000" mode should be used and "0" should be used
> > > >>> during
> > > >>> >> > > opening of a file. Or we need to remove "inode_checkflags()".
> > > >>> >> > >
> > > >>> >> > > Best regards,
> > > >>> >> > > Petro
> > > >>> >> > >
> > > >>> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> > > >>> >> > > <pe...@gmail.com>
> > > >>> >> > > пише:
> > > >>> >> > >
> > > >>> >> > >> I see. Thank you for the feedback. I will rework changes to
> > get
> > > >>> back
> > > >>> >> > >> read permissions.
> > > >>> >> > >>
> > > >>> >> > >> Best regards,
> > > >>> >> > >> Petro
> > > >>> >> > >>
> > > >>> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> > > >>> >> > >> <acassis@gmail.com
> > > >>> >> >
> > > >>> >> > >> пише:
> > > >>> >> > >> >
> > > >>> >> > >> > Hi Petro,
> > > >>> >> > >> >
> > > >>> >> > >> > The read permission is needed even when you just want to
> > open
> > > >>> >> > >> > a
> > > >>> >> file:
> > > >>> >> > >> >
> > > >>> >> > >> > $ vim noreadfile
> > > >>> >> > >> >
> > > >>> >> > >> > $ chmod 0000 noreadfile
> > > >>> >> > >> >
> > > >>> >> > >> > $ ls -l noreadfile
> > > >>> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> > > >>> >> > >> >
> > > >>> >> > >> > $ cat noreadfile
> > > >>> >> > >> > cat: noreadfile: Permission denied
> > > >>> >> > >> >
> > > >>> >> > >> > You can even try to create a C program just to open it,
> > and it
> > > >>> >> > >> > will
> > > >>> >> > >> > fail.
> > > >>> >> > >> >
> > > >>> >> > >> > See the man page of open function:
> > > >>> >> > >> >
> > > >>> >> > >> > The argument flags *must* include one of the
> > following
> > > >>> >> access
> > > >>> >> > >> > modes: O_RDONLY, O_WRONLY, or
> > > >>> >> > >> > O_RDWR. These request opening the file read-only,
> > > >>> >> write-only,
> > > >>> >> > >> > or read/write, respectively.
> > > >>> >> > >> >
> > > >>> >> > >> > This man page makes it clear you must include an access
> > mode,
> > > >>> >> > >> > but
> > > >>> >> > >> > I
> > > >>> >> > >> > passed 0 to the access mode flag of open() and it was
> > > >>> >> > >> > accepted,
> > > >>> >> > >> > but
> > > >>> >> > >> > when the file has permission 0000 it returns -EPERM:
> > "Failed
> > > >>> >> > >> > to
> > > >>> >> > >> > open
> > > >>> >> > >> > file: error -1"
> > > >>> >> > >> >
> > > >>> >> > >> > BR,
> > > >>> >> > >> >
> > > >>> >> > >> > Alan
> > > >>> >> > >> >
> > > >>> >> > >> > On 1/28/22, Petro Karashchenko <
> > petro.karashchenko@gmail.com>
> > > >>> >> wrote:
> > > >>> >> > >> > > Hello,
> > > >>> >> > >> > >
> > > >>> >> > >> > > Yes, but how does this relate to "0000" mode for
> > > >>> >> > "register_driver()"?
> > > >>> >> > >> > > Maybe you can describe some use case so it will become
> > more
> > > >>> >> > >> > > clear?
> > > >>> >> > >> > > Currently ioctl works fine if driver is registered with
> > > >>> >> > >> > > "0000"
> > > >>> >> > >> permission
> > > >>> >> > >> > > mode.
> > > >>> >> > >> > >
> > > >>> >> > >> > > Best regards,
> > > >>> >> > >> > > Petro
> > > >>> >> > >> > >
> > > >>> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> > > >>> >> > >> > > <xiaoxiang781216@gmail.com
> > > >>> >> >
> > > >>> >> > >> пише:
> > > >>> >> > >> > >>
> > > >>> >> > >> > >> If we want to do the correct permission check, the ioctl
> > > >>> >> > >> > >> handler
> > > >>> >> > >> needs to
> > > >>> >> > >> > >> check R/W bit by itself based on how the ioctl is
> > > >>> >> > >> > >> implemented.
> > > >>> >> > >> > >> Or follow up how Linux encode the needed permission into
> > > >>> >> > >> > >> each
> > > >>> >> > IOCTL:
> > > >>> >> > >> > >>
> > > >>> >> > >>
> > > >>> >> >
> > > >>> >>
> > > >>>
> > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > > >>> >> > >> > >> and let's VFS layer do the check for each driver.
> > > >>> >> > >> > >>
> > > >>> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > > >>> >> > >> > >> petro.karashchenko@gmail.com> wrote:
> > > >>> >> > >> > >>
> > > >>> >> > >> > >> > Hello team,
> > > >>> >> > >> > >> >
> > > >>> >> > >> > >> > Recently I have noticed that there are many places in
> > > >>> >> > >> > >> > code
> > > >>> >> where
> > > >>> >> > >> > >> > register_driver() is called with non-zero mode with
> > file
> > > >>> >> > operation
> > > >>> >> > >> > >> > structures that have neither read nor write APIs
> > > >>> implemented.
> > > >>> >> For
> > > >>> >> > >> > >> > example "ret = register_driver(path, &opamp_fops,
> > 0444,
> > > >>> >> > >> > >> > dev);"
> > > >>> >> > >> > >> > while
> > > >>> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> > > >>> >> "opamp_ioctl"
> > > >>> >> > >> > >> > implemented. I made a PR to fix it
> > > >>> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347
> > and
> > > >>> >> > >> > >> > change
> > > >>> >> > >> > >> > mode
> > > >>> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees
> > any
> > > >>> >> > drawback
> > > >>> >> > >> in
> > > >>> >> > >> > >> > such an approach? Maybe I'm missing something?
> > > >>> >> > >> > >> >
> > > >>> >> > >> > >> > Best regards,
> > > >>> >> > >> > >> > Petro
> > > >>> >> > >> > >> >
> > > >>> >> > >> > >
> > > >>> >> > >>
> > > >>> >> > >
> > > >>> >> >
> > > >>> >>
> > > >>> >
> > > >>>
> > > >>
> > > >
> > >
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Yes Jukka what you are saying is absolutely correct. The main item under
discussion are the drivers that are pure ioctl (that means do not have
neither read nor write handlers). Should RD or WR flag be passed to open
call in such case of or 0 should be passed.
Best regards,
Petro
On Fri, Apr 1, 2022, 6:54 PM Jukka Laitinen <ju...@iki.fi> wrote:
> Different posix implementations have different values for these flags, so
> I think it is ok not to have the same as what linux has.
>
> Posix (2017) specifies thar exactly one of the following is provided for
> open: O_EXEC, O_RDWR, O_RDONLY, O_SEARCH and O_WRONLY, and other flags are
> bitwise OR'd to that. The spec says nothing about that you can just give
> "0" afaik, it just happens to be that in Linux O_RDONLY happens to be 0.
>
> Maybe just fix the open having O_RDONLY in places where you really open
> the file as read-only, O_WRONLY for write only and O? That should be
> according to the standard at least.
>
> Just my 2 cents
>
> - Jukka
>
>
>
> Alan Carvalho de Assis kirjoitti perjantai 1. huhtikuuta 2022:
> > More about it here:
> >
> https://stackoverflow.com/questions/61923703/how-to-make-sense-of-o-rdonly-0
> >
> > So, I agree with the comment that said "calling it flag is misnomer
> > and misleading"
> >
> > On Unix/Linux O_RDONLY | O_WRONLY != O_RDWR, on NuttX is it explicitly
> this way:
> >
> > #define O_RDWR (O_RDOK|O_WROK) /* Open for both read & write access
> */
> >
> > Now, I'm also confuse about the right thing to do:
> >
> > 1) Fix include/fcntl.h to follow Unix/Linux
> > 2) Keep things we they are and don't accept opening a file if it
> > doesn't have RDONLY flag (and change all the drivers, event ioctl only
> > drivers, to include reading flag)
> > 3) Remove the flag checking.
> >
> > Probably we should do 1) because NuttX follows Unix/Linux approach,
> > although I agree that Unix/Linux are completely non-sense on this
> > subject, oflag should be a flag like it is on NuttX :-)
> >
> > BR,
> >
> > Alan
> >
> > On 4/1/22, Alan Carvalho de Assis <ac...@gmail.com> wrote:
> > > Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it:
> > >
> > > #define O_RDONLY 00000000
> > >
> > > So, in fact on Linux/Mac when we are opening a file with oflag 0 we
> > > are opening it for reading only.
> > >
> > > On NuttX the value 0 is not defined, O_RDONLY is 1:
> > >
> > > #define O_RDONLY (1 << 0)
> > >
> > > BR,
> > >
> > > Alan
> > >
> > > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > >> Hello,
> > >>
> > >> Here I'm talking not about driver registration permission, but more
> about
> > >> the "oflag" parameter to "open()" call.
> > >>
> > >> I just tried a quick example on MAC
> > >>
> > >> #include <fcntl.h>
> > >> #include <stdio.h>
> > >>
> > >> int main(void)
> > >> {
> > >> int fd = open("test.txt", 0);
> > >> if (fd < 0)
> > >> printf("A\n");
> > >> else
> > >> printf("B\n");
> > >>
> > >> return 0;
> > >> }
> > >>
> > >> The "B" is printed if the file exists. If you know the system that
> will
> > >> run
> > >> this sample code and will print "A", please let me know.
> > >>
> > >> Best regards,
> > >> Petro
> > >>
> > >> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <acassis@gmail.com
> >
> > >> пише:
> > >>
> > >>> I think the device file shouldn't be created with permission 000.
> > >>>
> > >>> Look inside your Linux /dev all device files have RW permission for
> > >>> root, some give only R for group and others.
> > >>>
> > >>> So, probably we need to fix the device register creation, not
> removing
> > >>> the flag check.
> > >>>
> > >>> BR,
> > >>>
> > >>> Alan
> > >>>
> > >>> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> > >>> > It's better to check ioctl callback too since ioctl means the
> driver
> > >>> > has
> > >>> > the compatibility of read(i)and write(o).
> > >>> >
> > >>> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> > >>> > petro.karashchenko@gmail.com> wrote:
> > >>> >
> > >>> >> So Alan do you suggest to remove inode_checkflags?
> > >>> >>
> > >>> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis
> > >>> >> <ac...@gmail.com>
> > >>> >> wrote:
> > >>> >>
> > >>> >> > Hi Petro,
> > >>> >> >
> > >>> >> > I saw your PR #1117 but I think opening a device file with flag
> 0
> > >>> >> > is
> > >>> >> > not correct, please see the open man-pages:
> > >>> >> >
> > >>> >> > alan@dev:/tmp$ man 2 open
> > >>> >> >
> > >>> >> > The argument flags must include one of the following
> > >>> >> > access
> > >>> >> > modes: O_RDONLY, O_WRONLY, or
> > >>> >> > O_RDWR. These request opening the file read-only,
> > >>> >> > write-only,
> > >>> >> > or read/write, respectively.
> > >>> >> >
> > >>> >> > Also the opengroup say something similar:
> > >>> >> >
> > >>> >> >
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> > >>> >> >
> > >>> >> > "Values for oflag are constructed by a bitwise-inclusive OR of
> > >>> >> > flags
> > >>> >> > from the following list, defined in <fcntl.h>. Applications
> shall
> > >>> >> > specify exactly one of the first five values (file access modes)
> > >>> >> > below
> > >>> >> > in the value of oflag:"
> > >>> >> >
> > >>> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
> > >>> >> > according
> > >>> >> > to RFC2119 they are equivalents:
> > >>> >> >
> > >>> >> > https://www.ietf.org/rfc/rfc2119.txt
> > >>> >> >
> > >>> >> > BR,
> > >>> >> >
> > >>> >> > Alan
> > >>> >> >
> > >>> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com>
> wrote:
> > >>> >> > > Hi,
> > >>> >> > >
> > >>> >> > > I want to resume this thread again because after reexamined
> code
> > >>> >> > carefully
> > >>> >> > > I found that VFS layer has an API
> > >>> >> > >
> > >>> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> > >>> >> > > {
> > >>> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> > >>> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> > >>> >> > > {
> > >>> >> > > return -EACCES;
> > >>> >> > > }
> > >>> >> > > else
> > >>> >> > > {
> > >>> >> > > return OK;
> > >>> >> > > }
> > >>> >> > > }
> > >>> >> > >
> > >>> >> > > That checks if read and write handlers are available, so all
> our
> > >>> >> > discussion
> > >>> >> > > about R/W mode for IOCTL does not make any sense. We either
> need
> > >>> >> > > to
> > >>> >> > remove
> > >>> >> > > this check or register VFS nodes with proper permissions and
> open
> > >>> >> > > files
> > >>> >> > > with correct flags. So if the driver does not have neither
> read
> > >>> >> > > nor
> > >>> >> write
> > >>> >> > > handlers the "0000" mode should be used and "0" should be used
> > >>> during
> > >>> >> > > opening of a file. Or we need to remove "inode_checkflags()".
> > >>> >> > >
> > >>> >> > > Best regards,
> > >>> >> > > Petro
> > >>> >> > >
> > >>> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> > >>> >> > > <pe...@gmail.com>
> > >>> >> > > пише:
> > >>> >> > >
> > >>> >> > >> I see. Thank you for the feedback. I will rework changes to
> get
> > >>> back
> > >>> >> > >> read permissions.
> > >>> >> > >>
> > >>> >> > >> Best regards,
> > >>> >> > >> Petro
> > >>> >> > >>
> > >>> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> > >>> >> > >> <acassis@gmail.com
> > >>> >> >
> > >>> >> > >> пише:
> > >>> >> > >> >
> > >>> >> > >> > Hi Petro,
> > >>> >> > >> >
> > >>> >> > >> > The read permission is needed even when you just want to
> open
> > >>> >> > >> > a
> > >>> >> file:
> > >>> >> > >> >
> > >>> >> > >> > $ vim noreadfile
> > >>> >> > >> >
> > >>> >> > >> > $ chmod 0000 noreadfile
> > >>> >> > >> >
> > >>> >> > >> > $ ls -l noreadfile
> > >>> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> > >>> >> > >> >
> > >>> >> > >> > $ cat noreadfile
> > >>> >> > >> > cat: noreadfile: Permission denied
> > >>> >> > >> >
> > >>> >> > >> > You can even try to create a C program just to open it,
> and it
> > >>> >> > >> > will
> > >>> >> > >> > fail.
> > >>> >> > >> >
> > >>> >> > >> > See the man page of open function:
> > >>> >> > >> >
> > >>> >> > >> > The argument flags *must* include one of the
> following
> > >>> >> access
> > >>> >> > >> > modes: O_RDONLY, O_WRONLY, or
> > >>> >> > >> > O_RDWR. These request opening the file read-only,
> > >>> >> write-only,
> > >>> >> > >> > or read/write, respectively.
> > >>> >> > >> >
> > >>> >> > >> > This man page makes it clear you must include an access
> mode,
> > >>> >> > >> > but
> > >>> >> > >> > I
> > >>> >> > >> > passed 0 to the access mode flag of open() and it was
> > >>> >> > >> > accepted,
> > >>> >> > >> > but
> > >>> >> > >> > when the file has permission 0000 it returns -EPERM:
> "Failed
> > >>> >> > >> > to
> > >>> >> > >> > open
> > >>> >> > >> > file: error -1"
> > >>> >> > >> >
> > >>> >> > >> > BR,
> > >>> >> > >> >
> > >>> >> > >> > Alan
> > >>> >> > >> >
> > >>> >> > >> > On 1/28/22, Petro Karashchenko <
> petro.karashchenko@gmail.com>
> > >>> >> wrote:
> > >>> >> > >> > > Hello,
> > >>> >> > >> > >
> > >>> >> > >> > > Yes, but how does this relate to "0000" mode for
> > >>> >> > "register_driver()"?
> > >>> >> > >> > > Maybe you can describe some use case so it will become
> more
> > >>> >> > >> > > clear?
> > >>> >> > >> > > Currently ioctl works fine if driver is registered with
> > >>> >> > >> > > "0000"
> > >>> >> > >> permission
> > >>> >> > >> > > mode.
> > >>> >> > >> > >
> > >>> >> > >> > > Best regards,
> > >>> >> > >> > > Petro
> > >>> >> > >> > >
> > >>> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> > >>> >> > >> > > <xiaoxiang781216@gmail.com
> > >>> >> >
> > >>> >> > >> пише:
> > >>> >> > >> > >>
> > >>> >> > >> > >> If we want to do the correct permission check, the ioctl
> > >>> >> > >> > >> handler
> > >>> >> > >> needs to
> > >>> >> > >> > >> check R/W bit by itself based on how the ioctl is
> > >>> >> > >> > >> implemented.
> > >>> >> > >> > >> Or follow up how Linux encode the needed permission into
> > >>> >> > >> > >> each
> > >>> >> > IOCTL:
> > >>> >> > >> > >>
> > >>> >> > >>
> > >>> >> >
> > >>> >>
> > >>>
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > >>> >> > >> > >> and let's VFS layer do the check for each driver.
> > >>> >> > >> > >>
> > >>> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > >>> >> > >> > >> petro.karashchenko@gmail.com> wrote:
> > >>> >> > >> > >>
> > >>> >> > >> > >> > Hello team,
> > >>> >> > >> > >> >
> > >>> >> > >> > >> > Recently I have noticed that there are many places in
> > >>> >> > >> > >> > code
> > >>> >> where
> > >>> >> > >> > >> > register_driver() is called with non-zero mode with
> file
> > >>> >> > operation
> > >>> >> > >> > >> > structures that have neither read nor write APIs
> > >>> implemented.
> > >>> >> For
> > >>> >> > >> > >> > example "ret = register_driver(path, &opamp_fops,
> 0444,
> > >>> >> > >> > >> > dev);"
> > >>> >> > >> > >> > while
> > >>> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> > >>> >> "opamp_ioctl"
> > >>> >> > >> > >> > implemented. I made a PR to fix it
> > >>> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347
> and
> > >>> >> > >> > >> > change
> > >>> >> > >> > >> > mode
> > >>> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees
> any
> > >>> >> > drawback
> > >>> >> > >> in
> > >>> >> > >> > >> > such an approach? Maybe I'm missing something?
> > >>> >> > >> > >> >
> > >>> >> > >> > >> > Best regards,
> > >>> >> > >> > >> > Petro
> > >>> >> > >> > >> >
> > >>> >> > >> > >
> > >>> >> > >>
> > >>> >> > >
> > >>> >> >
> > >>> >>
> > >>> >
> > >>>
> > >>
> > >
> >
Re: register_driver with 0000 mode
Posted by Jukka Laitinen <ju...@iki.fi>.
Different posix implementations have different values for these flags, so I think it is ok not to have the same as what linux has.
Posix (2017) specifies thar exactly one of the following is provided for open: O_EXEC, O_RDWR, O_RDONLY, O_SEARCH and O_WRONLY, and other flags are bitwise OR'd to that. The spec says nothing about that you can just give "0" afaik, it just happens to be that in Linux O_RDONLY happens to be 0.
Maybe just fix the open having O_RDONLY in places where you really open the file as read-only, O_WRONLY for write only and O? That should be according to the standard at least.
Just my 2 cents
- Jukka
Alan Carvalho de Assis kirjoitti perjantai 1. huhtikuuta 2022:
> More about it here:
> https://stackoverflow.com/questions/61923703/how-to-make-sense-of-o-rdonly-0
>
> So, I agree with the comment that said "calling it flag is misnomer
> and misleading"
>
> On Unix/Linux O_RDONLY | O_WRONLY != O_RDWR, on NuttX is it explicitly this way:
>
> #define O_RDWR (O_RDOK|O_WROK) /* Open for both read & write access */
>
> Now, I'm also confuse about the right thing to do:
>
> 1) Fix include/fcntl.h to follow Unix/Linux
> 2) Keep things we they are and don't accept opening a file if it
> doesn't have RDONLY flag (and change all the drivers, event ioctl only
> drivers, to include reading flag)
> 3) Remove the flag checking.
>
> Probably we should do 1) because NuttX follows Unix/Linux approach,
> although I agree that Unix/Linux are completely non-sense on this
> subject, oflag should be a flag like it is on NuttX :-)
>
> BR,
>
> Alan
>
> On 4/1/22, Alan Carvalho de Assis <ac...@gmail.com> wrote:
> > Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it:
> >
> > #define O_RDONLY 00000000
> >
> > So, in fact on Linux/Mac when we are opening a file with oflag 0 we
> > are opening it for reading only.
> >
> > On NuttX the value 0 is not defined, O_RDONLY is 1:
> >
> > #define O_RDONLY (1 << 0)
> >
> > BR,
> >
> > Alan
> >
> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> >> Hello,
> >>
> >> Here I'm talking not about driver registration permission, but more about
> >> the "oflag" parameter to "open()" call.
> >>
> >> I just tried a quick example on MAC
> >>
> >> #include <fcntl.h>
> >> #include <stdio.h>
> >>
> >> int main(void)
> >> {
> >> int fd = open("test.txt", 0);
> >> if (fd < 0)
> >> printf("A\n");
> >> else
> >> printf("B\n");
> >>
> >> return 0;
> >> }
> >>
> >> The "B" is printed if the file exists. If you know the system that will
> >> run
> >> this sample code and will print "A", please let me know.
> >>
> >> Best regards,
> >> Petro
> >>
> >> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <ac...@gmail.com>
> >> пише:
> >>
> >>> I think the device file shouldn't be created with permission 000.
> >>>
> >>> Look inside your Linux /dev all device files have RW permission for
> >>> root, some give only R for group and others.
> >>>
> >>> So, probably we need to fix the device register creation, not removing
> >>> the flag check.
> >>>
> >>> BR,
> >>>
> >>> Alan
> >>>
> >>> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> >>> > It's better to check ioctl callback too since ioctl means the driver
> >>> > has
> >>> > the compatibility of read(i)and write(o).
> >>> >
> >>> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> >>> > petro.karashchenko@gmail.com> wrote:
> >>> >
> >>> >> So Alan do you suggest to remove inode_checkflags?
> >>> >>
> >>> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis
> >>> >> <ac...@gmail.com>
> >>> >> wrote:
> >>> >>
> >>> >> > Hi Petro,
> >>> >> >
> >>> >> > I saw your PR #1117 but I think opening a device file with flag 0
> >>> >> > is
> >>> >> > not correct, please see the open man-pages:
> >>> >> >
> >>> >> > alan@dev:/tmp$ man 2 open
> >>> >> >
> >>> >> > The argument flags must include one of the following
> >>> >> > access
> >>> >> > modes: O_RDONLY, O_WRONLY, or
> >>> >> > O_RDWR. These request opening the file read-only,
> >>> >> > write-only,
> >>> >> > or read/write, respectively.
> >>> >> >
> >>> >> > Also the opengroup say something similar:
> >>> >> >
> >>> >> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> >>> >> >
> >>> >> > "Values for oflag are constructed by a bitwise-inclusive OR of
> >>> >> > flags
> >>> >> > from the following list, defined in <fcntl.h>. Applications shall
> >>> >> > specify exactly one of the first five values (file access modes)
> >>> >> > below
> >>> >> > in the value of oflag:"
> >>> >> >
> >>> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
> >>> >> > according
> >>> >> > to RFC2119 they are equivalents:
> >>> >> >
> >>> >> > https://www.ietf.org/rfc/rfc2119.txt
> >>> >> >
> >>> >> > BR,
> >>> >> >
> >>> >> > Alan
> >>> >> >
> >>> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> >>> >> > > Hi,
> >>> >> > >
> >>> >> > > I want to resume this thread again because after reexamined code
> >>> >> > carefully
> >>> >> > > I found that VFS layer has an API
> >>> >> > >
> >>> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> >>> >> > > {
> >>> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> >>> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> >>> >> > > {
> >>> >> > > return -EACCES;
> >>> >> > > }
> >>> >> > > else
> >>> >> > > {
> >>> >> > > return OK;
> >>> >> > > }
> >>> >> > > }
> >>> >> > >
> >>> >> > > That checks if read and write handlers are available, so all our
> >>> >> > discussion
> >>> >> > > about R/W mode for IOCTL does not make any sense. We either need
> >>> >> > > to
> >>> >> > remove
> >>> >> > > this check or register VFS nodes with proper permissions and open
> >>> >> > > files
> >>> >> > > with correct flags. So if the driver does not have neither read
> >>> >> > > nor
> >>> >> write
> >>> >> > > handlers the "0000" mode should be used and "0" should be used
> >>> during
> >>> >> > > opening of a file. Or we need to remove "inode_checkflags()".
> >>> >> > >
> >>> >> > > Best regards,
> >>> >> > > Petro
> >>> >> > >
> >>> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> >>> >> > > <pe...@gmail.com>
> >>> >> > > пише:
> >>> >> > >
> >>> >> > >> I see. Thank you for the feedback. I will rework changes to get
> >>> back
> >>> >> > >> read permissions.
> >>> >> > >>
> >>> >> > >> Best regards,
> >>> >> > >> Petro
> >>> >> > >>
> >>> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> >>> >> > >> <acassis@gmail.com
> >>> >> >
> >>> >> > >> пише:
> >>> >> > >> >
> >>> >> > >> > Hi Petro,
> >>> >> > >> >
> >>> >> > >> > The read permission is needed even when you just want to open
> >>> >> > >> > a
> >>> >> file:
> >>> >> > >> >
> >>> >> > >> > $ vim noreadfile
> >>> >> > >> >
> >>> >> > >> > $ chmod 0000 noreadfile
> >>> >> > >> >
> >>> >> > >> > $ ls -l noreadfile
> >>> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> >>> >> > >> >
> >>> >> > >> > $ cat noreadfile
> >>> >> > >> > cat: noreadfile: Permission denied
> >>> >> > >> >
> >>> >> > >> > You can even try to create a C program just to open it, and it
> >>> >> > >> > will
> >>> >> > >> > fail.
> >>> >> > >> >
> >>> >> > >> > See the man page of open function:
> >>> >> > >> >
> >>> >> > >> > The argument flags *must* include one of the following
> >>> >> access
> >>> >> > >> > modes: O_RDONLY, O_WRONLY, or
> >>> >> > >> > O_RDWR. These request opening the file read-only,
> >>> >> write-only,
> >>> >> > >> > or read/write, respectively.
> >>> >> > >> >
> >>> >> > >> > This man page makes it clear you must include an access mode,
> >>> >> > >> > but
> >>> >> > >> > I
> >>> >> > >> > passed 0 to the access mode flag of open() and it was
> >>> >> > >> > accepted,
> >>> >> > >> > but
> >>> >> > >> > when the file has permission 0000 it returns -EPERM: "Failed
> >>> >> > >> > to
> >>> >> > >> > open
> >>> >> > >> > file: error -1"
> >>> >> > >> >
> >>> >> > >> > BR,
> >>> >> > >> >
> >>> >> > >> > Alan
> >>> >> > >> >
> >>> >> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
> >>> >> wrote:
> >>> >> > >> > > Hello,
> >>> >> > >> > >
> >>> >> > >> > > Yes, but how does this relate to "0000" mode for
> >>> >> > "register_driver()"?
> >>> >> > >> > > Maybe you can describe some use case so it will become more
> >>> >> > >> > > clear?
> >>> >> > >> > > Currently ioctl works fine if driver is registered with
> >>> >> > >> > > "0000"
> >>> >> > >> permission
> >>> >> > >> > > mode.
> >>> >> > >> > >
> >>> >> > >> > > Best regards,
> >>> >> > >> > > Petro
> >>> >> > >> > >
> >>> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> >>> >> > >> > > <xiaoxiang781216@gmail.com
> >>> >> >
> >>> >> > >> пише:
> >>> >> > >> > >>
> >>> >> > >> > >> If we want to do the correct permission check, the ioctl
> >>> >> > >> > >> handler
> >>> >> > >> needs to
> >>> >> > >> > >> check R/W bit by itself based on how the ioctl is
> >>> >> > >> > >> implemented.
> >>> >> > >> > >> Or follow up how Linux encode the needed permission into
> >>> >> > >> > >> each
> >>> >> > IOCTL:
> >>> >> > >> > >>
> >>> >> > >>
> >>> >> >
> >>> >>
> >>> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> >>> >> > >> > >> and let's VFS layer do the check for each driver.
> >>> >> > >> > >>
> >>> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> >>> >> > >> > >> petro.karashchenko@gmail.com> wrote:
> >>> >> > >> > >>
> >>> >> > >> > >> > Hello team,
> >>> >> > >> > >> >
> >>> >> > >> > >> > Recently I have noticed that there are many places in
> >>> >> > >> > >> > code
> >>> >> where
> >>> >> > >> > >> > register_driver() is called with non-zero mode with file
> >>> >> > operation
> >>> >> > >> > >> > structures that have neither read nor write APIs
> >>> implemented.
> >>> >> For
> >>> >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
> >>> >> > >> > >> > dev);"
> >>> >> > >> > >> > while
> >>> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> >>> >> "opamp_ioctl"
> >>> >> > >> > >> > implemented. I made a PR to fix it
> >>> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
> >>> >> > >> > >> > change
> >>> >> > >> > >> > mode
> >>> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
> >>> >> > drawback
> >>> >> > >> in
> >>> >> > >> > >> > such an approach? Maybe I'm missing something?
> >>> >> > >> > >> >
> >>> >> > >> > >> > Best regards,
> >>> >> > >> > >> > Petro
> >>> >> > >> > >> >
> >>> >> > >> > >
> >>> >> > >>
> >>> >> > >
> >>> >> >
> >>> >>
> >>> >
> >>>
> >>
> >
>
Re: register_driver with 0000 mode
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
More about it here:
https://stackoverflow.com/questions/61923703/how-to-make-sense-of-o-rdonly-0
So, I agree with the comment that said "calling it flag is misnomer
and misleading"
On Unix/Linux O_RDONLY | O_WRONLY != O_RDWR, on NuttX is it explicitly this way:
#define O_RDWR (O_RDOK|O_WROK) /* Open for both read & write access */
Now, I'm also confuse about the right thing to do:
1) Fix include/fcntl.h to follow Unix/Linux
2) Keep things we they are and don't accept opening a file if it
doesn't have RDONLY flag (and change all the drivers, event ioctl only
drivers, to include reading flag)
3) Remove the flag checking.
Probably we should do 1) because NuttX follows Unix/Linux approach,
although I agree that Unix/Linux are completely non-sense on this
subject, oflag should be a flag like it is on NuttX :-)
BR,
Alan
On 4/1/22, Alan Carvalho de Assis <ac...@gmail.com> wrote:
> Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it:
>
> #define O_RDONLY 00000000
>
> So, in fact on Linux/Mac when we are opening a file with oflag 0 we
> are opening it for reading only.
>
> On NuttX the value 0 is not defined, O_RDONLY is 1:
>
> #define O_RDONLY (1 << 0)
>
> BR,
>
> Alan
>
> On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
>> Hello,
>>
>> Here I'm talking not about driver registration permission, but more about
>> the "oflag" parameter to "open()" call.
>>
>> I just tried a quick example on MAC
>>
>> #include <fcntl.h>
>> #include <stdio.h>
>>
>> int main(void)
>> {
>> int fd = open("test.txt", 0);
>> if (fd < 0)
>> printf("A\n");
>> else
>> printf("B\n");
>>
>> return 0;
>> }
>>
>> The "B" is printed if the file exists. If you know the system that will
>> run
>> this sample code and will print "A", please let me know.
>>
>> Best regards,
>> Petro
>>
>> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <ac...@gmail.com>
>> пише:
>>
>>> I think the device file shouldn't be created with permission 000.
>>>
>>> Look inside your Linux /dev all device files have RW permission for
>>> root, some give only R for group and others.
>>>
>>> So, probably we need to fix the device register creation, not removing
>>> the flag check.
>>>
>>> BR,
>>>
>>> Alan
>>>
>>> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
>>> > It's better to check ioctl callback too since ioctl means the driver
>>> > has
>>> > the compatibility of read(i)and write(o).
>>> >
>>> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
>>> > petro.karashchenko@gmail.com> wrote:
>>> >
>>> >> So Alan do you suggest to remove inode_checkflags?
>>> >>
>>> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis
>>> >> <ac...@gmail.com>
>>> >> wrote:
>>> >>
>>> >> > Hi Petro,
>>> >> >
>>> >> > I saw your PR #1117 but I think opening a device file with flag 0
>>> >> > is
>>> >> > not correct, please see the open man-pages:
>>> >> >
>>> >> > alan@dev:/tmp$ man 2 open
>>> >> >
>>> >> > The argument flags must include one of the following
>>> >> > access
>>> >> > modes: O_RDONLY, O_WRONLY, or
>>> >> > O_RDWR. These request opening the file read-only,
>>> >> > write-only,
>>> >> > or read/write, respectively.
>>> >> >
>>> >> > Also the opengroup say something similar:
>>> >> >
>>> >> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
>>> >> >
>>> >> > "Values for oflag are constructed by a bitwise-inclusive OR of
>>> >> > flags
>>> >> > from the following list, defined in <fcntl.h>. Applications shall
>>> >> > specify exactly one of the first five values (file access modes)
>>> >> > below
>>> >> > in the value of oflag:"
>>> >> >
>>> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
>>> >> > according
>>> >> > to RFC2119 they are equivalents:
>>> >> >
>>> >> > https://www.ietf.org/rfc/rfc2119.txt
>>> >> >
>>> >> > BR,
>>> >> >
>>> >> > Alan
>>> >> >
>>> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
>>> >> > > Hi,
>>> >> > >
>>> >> > > I want to resume this thread again because after reexamined code
>>> >> > carefully
>>> >> > > I found that VFS layer has an API
>>> >> > >
>>> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
>>> >> > > {
>>> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
>>> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
>>> >> > > {
>>> >> > > return -EACCES;
>>> >> > > }
>>> >> > > else
>>> >> > > {
>>> >> > > return OK;
>>> >> > > }
>>> >> > > }
>>> >> > >
>>> >> > > That checks if read and write handlers are available, so all our
>>> >> > discussion
>>> >> > > about R/W mode for IOCTL does not make any sense. We either need
>>> >> > > to
>>> >> > remove
>>> >> > > this check or register VFS nodes with proper permissions and open
>>> >> > > files
>>> >> > > with correct flags. So if the driver does not have neither read
>>> >> > > nor
>>> >> write
>>> >> > > handlers the "0000" mode should be used and "0" should be used
>>> during
>>> >> > > opening of a file. Or we need to remove "inode_checkflags()".
>>> >> > >
>>> >> > > Best regards,
>>> >> > > Petro
>>> >> > >
>>> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
>>> >> > > <pe...@gmail.com>
>>> >> > > пише:
>>> >> > >
>>> >> > >> I see. Thank you for the feedback. I will rework changes to get
>>> back
>>> >> > >> read permissions.
>>> >> > >>
>>> >> > >> Best regards,
>>> >> > >> Petro
>>> >> > >>
>>> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
>>> >> > >> <acassis@gmail.com
>>> >> >
>>> >> > >> пише:
>>> >> > >> >
>>> >> > >> > Hi Petro,
>>> >> > >> >
>>> >> > >> > The read permission is needed even when you just want to open
>>> >> > >> > a
>>> >> file:
>>> >> > >> >
>>> >> > >> > $ vim noreadfile
>>> >> > >> >
>>> >> > >> > $ chmod 0000 noreadfile
>>> >> > >> >
>>> >> > >> > $ ls -l noreadfile
>>> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
>>> >> > >> >
>>> >> > >> > $ cat noreadfile
>>> >> > >> > cat: noreadfile: Permission denied
>>> >> > >> >
>>> >> > >> > You can even try to create a C program just to open it, and it
>>> >> > >> > will
>>> >> > >> > fail.
>>> >> > >> >
>>> >> > >> > See the man page of open function:
>>> >> > >> >
>>> >> > >> > The argument flags *must* include one of the following
>>> >> access
>>> >> > >> > modes: O_RDONLY, O_WRONLY, or
>>> >> > >> > O_RDWR. These request opening the file read-only,
>>> >> write-only,
>>> >> > >> > or read/write, respectively.
>>> >> > >> >
>>> >> > >> > This man page makes it clear you must include an access mode,
>>> >> > >> > but
>>> >> > >> > I
>>> >> > >> > passed 0 to the access mode flag of open() and it was
>>> >> > >> > accepted,
>>> >> > >> > but
>>> >> > >> > when the file has permission 0000 it returns -EPERM: "Failed
>>> >> > >> > to
>>> >> > >> > open
>>> >> > >> > file: error -1"
>>> >> > >> >
>>> >> > >> > BR,
>>> >> > >> >
>>> >> > >> > Alan
>>> >> > >> >
>>> >> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
>>> >> wrote:
>>> >> > >> > > Hello,
>>> >> > >> > >
>>> >> > >> > > Yes, but how does this relate to "0000" mode for
>>> >> > "register_driver()"?
>>> >> > >> > > Maybe you can describe some use case so it will become more
>>> >> > >> > > clear?
>>> >> > >> > > Currently ioctl works fine if driver is registered with
>>> >> > >> > > "0000"
>>> >> > >> permission
>>> >> > >> > > mode.
>>> >> > >> > >
>>> >> > >> > > Best regards,
>>> >> > >> > > Petro
>>> >> > >> > >
>>> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
>>> >> > >> > > <xiaoxiang781216@gmail.com
>>> >> >
>>> >> > >> пише:
>>> >> > >> > >>
>>> >> > >> > >> If we want to do the correct permission check, the ioctl
>>> >> > >> > >> handler
>>> >> > >> needs to
>>> >> > >> > >> check R/W bit by itself based on how the ioctl is
>>> >> > >> > >> implemented.
>>> >> > >> > >> Or follow up how Linux encode the needed permission into
>>> >> > >> > >> each
>>> >> > IOCTL:
>>> >> > >> > >>
>>> >> > >>
>>> >> >
>>> >>
>>> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
>>> >> > >> > >> and let's VFS layer do the check for each driver.
>>> >> > >> > >>
>>> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
>>> >> > >> > >> petro.karashchenko@gmail.com> wrote:
>>> >> > >> > >>
>>> >> > >> > >> > Hello team,
>>> >> > >> > >> >
>>> >> > >> > >> > Recently I have noticed that there are many places in
>>> >> > >> > >> > code
>>> >> where
>>> >> > >> > >> > register_driver() is called with non-zero mode with file
>>> >> > operation
>>> >> > >> > >> > structures that have neither read nor write APIs
>>> implemented.
>>> >> For
>>> >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
>>> >> > >> > >> > dev);"
>>> >> > >> > >> > while
>>> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
>>> >> "opamp_ioctl"
>>> >> > >> > >> > implemented. I made a PR to fix it
>>> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
>>> >> > >> > >> > change
>>> >> > >> > >> > mode
>>> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
>>> >> > drawback
>>> >> > >> in
>>> >> > >> > >> > such an approach? Maybe I'm missing something?
>>> >> > >> > >> >
>>> >> > >> > >> > Best regards,
>>> >> > >> > >> > Petro
>>> >> > >> > >> >
>>> >> > >> > >
>>> >> > >>
>>> >> > >
>>> >> >
>>> >>
>>> >
>>>
>>
>
Re: register_driver with 0000 mode
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it:
#define O_RDONLY 00000000
So, in fact on Linux/Mac when we are opening a file with oflag 0 we
are opening it for reading only.
On NuttX the value 0 is not defined, O_RDONLY is 1:
#define O_RDONLY (1 << 0)
BR,
Alan
On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> Hello,
>
> Here I'm talking not about driver registration permission, but more about
> the "oflag" parameter to "open()" call.
>
> I just tried a quick example on MAC
>
> #include <fcntl.h>
> #include <stdio.h>
>
> int main(void)
> {
> int fd = open("test.txt", 0);
> if (fd < 0)
> printf("A\n");
> else
> printf("B\n");
>
> return 0;
> }
>
> The "B" is printed if the file exists. If you know the system that will run
> this sample code and will print "A", please let me know.
>
> Best regards,
> Petro
>
> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <ac...@gmail.com>
> пише:
>
>> I think the device file shouldn't be created with permission 000.
>>
>> Look inside your Linux /dev all device files have RW permission for
>> root, some give only R for group and others.
>>
>> So, probably we need to fix the device register creation, not removing
>> the flag check.
>>
>> BR,
>>
>> Alan
>>
>> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
>> > It's better to check ioctl callback too since ioctl means the driver
>> > has
>> > the compatibility of read(i)and write(o).
>> >
>> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
>> > petro.karashchenko@gmail.com> wrote:
>> >
>> >> So Alan do you suggest to remove inode_checkflags?
>> >>
>> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis
>> >> <ac...@gmail.com>
>> >> wrote:
>> >>
>> >> > Hi Petro,
>> >> >
>> >> > I saw your PR #1117 but I think opening a device file with flag 0 is
>> >> > not correct, please see the open man-pages:
>> >> >
>> >> > alan@dev:/tmp$ man 2 open
>> >> >
>> >> > The argument flags must include one of the following access
>> >> > modes: O_RDONLY, O_WRONLY, or
>> >> > O_RDWR. These request opening the file read-only,
>> >> > write-only,
>> >> > or read/write, respectively.
>> >> >
>> >> > Also the opengroup say something similar:
>> >> >
>> >> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
>> >> >
>> >> > "Values for oflag are constructed by a bitwise-inclusive OR of flags
>> >> > from the following list, defined in <fcntl.h>. Applications shall
>> >> > specify exactly one of the first five values (file access modes)
>> >> > below
>> >> > in the value of oflag:"
>> >> >
>> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
>> >> > according
>> >> > to RFC2119 they are equivalents:
>> >> >
>> >> > https://www.ietf.org/rfc/rfc2119.txt
>> >> >
>> >> > BR,
>> >> >
>> >> > Alan
>> >> >
>> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
>> >> > > Hi,
>> >> > >
>> >> > > I want to resume this thread again because after reexamined code
>> >> > carefully
>> >> > > I found that VFS layer has an API
>> >> > >
>> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
>> >> > > {
>> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
>> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
>> >> > > {
>> >> > > return -EACCES;
>> >> > > }
>> >> > > else
>> >> > > {
>> >> > > return OK;
>> >> > > }
>> >> > > }
>> >> > >
>> >> > > That checks if read and write handlers are available, so all our
>> >> > discussion
>> >> > > about R/W mode for IOCTL does not make any sense. We either need
>> >> > > to
>> >> > remove
>> >> > > this check or register VFS nodes with proper permissions and open
>> >> > > files
>> >> > > with correct flags. So if the driver does not have neither read
>> >> > > nor
>> >> write
>> >> > > handlers the "0000" mode should be used and "0" should be used
>> during
>> >> > > opening of a file. Or we need to remove "inode_checkflags()".
>> >> > >
>> >> > > Best regards,
>> >> > > Petro
>> >> > >
>> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
>> >> > > <pe...@gmail.com>
>> >> > > пише:
>> >> > >
>> >> > >> I see. Thank you for the feedback. I will rework changes to get
>> back
>> >> > >> read permissions.
>> >> > >>
>> >> > >> Best regards,
>> >> > >> Petro
>> >> > >>
>> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
>> >> > >> <acassis@gmail.com
>> >> >
>> >> > >> пише:
>> >> > >> >
>> >> > >> > Hi Petro,
>> >> > >> >
>> >> > >> > The read permission is needed even when you just want to open a
>> >> file:
>> >> > >> >
>> >> > >> > $ vim noreadfile
>> >> > >> >
>> >> > >> > $ chmod 0000 noreadfile
>> >> > >> >
>> >> > >> > $ ls -l noreadfile
>> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
>> >> > >> >
>> >> > >> > $ cat noreadfile
>> >> > >> > cat: noreadfile: Permission denied
>> >> > >> >
>> >> > >> > You can even try to create a C program just to open it, and it
>> >> > >> > will
>> >> > >> > fail.
>> >> > >> >
>> >> > >> > See the man page of open function:
>> >> > >> >
>> >> > >> > The argument flags *must* include one of the following
>> >> access
>> >> > >> > modes: O_RDONLY, O_WRONLY, or
>> >> > >> > O_RDWR. These request opening the file read-only,
>> >> write-only,
>> >> > >> > or read/write, respectively.
>> >> > >> >
>> >> > >> > This man page makes it clear you must include an access mode,
>> >> > >> > but
>> >> > >> > I
>> >> > >> > passed 0 to the access mode flag of open() and it was accepted,
>> >> > >> > but
>> >> > >> > when the file has permission 0000 it returns -EPERM: "Failed to
>> >> > >> > open
>> >> > >> > file: error -1"
>> >> > >> >
>> >> > >> > BR,
>> >> > >> >
>> >> > >> > Alan
>> >> > >> >
>> >> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
>> >> wrote:
>> >> > >> > > Hello,
>> >> > >> > >
>> >> > >> > > Yes, but how does this relate to "0000" mode for
>> >> > "register_driver()"?
>> >> > >> > > Maybe you can describe some use case so it will become more
>> >> > >> > > clear?
>> >> > >> > > Currently ioctl works fine if driver is registered with
>> >> > >> > > "0000"
>> >> > >> permission
>> >> > >> > > mode.
>> >> > >> > >
>> >> > >> > > Best regards,
>> >> > >> > > Petro
>> >> > >> > >
>> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
>> >> > >> > > <xiaoxiang781216@gmail.com
>> >> >
>> >> > >> пише:
>> >> > >> > >>
>> >> > >> > >> If we want to do the correct permission check, the ioctl
>> >> > >> > >> handler
>> >> > >> needs to
>> >> > >> > >> check R/W bit by itself based on how the ioctl is
>> >> > >> > >> implemented.
>> >> > >> > >> Or follow up how Linux encode the needed permission into
>> >> > >> > >> each
>> >> > IOCTL:
>> >> > >> > >>
>> >> > >>
>> >> >
>> >>
>> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
>> >> > >> > >> and let's VFS layer do the check for each driver.
>> >> > >> > >>
>> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
>> >> > >> > >> petro.karashchenko@gmail.com> wrote:
>> >> > >> > >>
>> >> > >> > >> > Hello team,
>> >> > >> > >> >
>> >> > >> > >> > Recently I have noticed that there are many places in code
>> >> where
>> >> > >> > >> > register_driver() is called with non-zero mode with file
>> >> > operation
>> >> > >> > >> > structures that have neither read nor write APIs
>> implemented.
>> >> For
>> >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
>> >> > >> > >> > dev);"
>> >> > >> > >> > while
>> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
>> >> "opamp_ioctl"
>> >> > >> > >> > implemented. I made a PR to fix it
>> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
>> >> > >> > >> > change
>> >> > >> > >> > mode
>> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
>> >> > drawback
>> >> > >> in
>> >> > >> > >> > such an approach? Maybe I'm missing something?
>> >> > >> > >> >
>> >> > >> > >> > Best regards,
>> >> > >> > >> > Petro
>> >> > >> > >> >
>> >> > >> > >
>> >> > >>
>> >> > >
>> >> >
>> >>
>> >
>>
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Ok. I will update the code example and come back later with some answers.
On Fri, Apr 1, 2022, 5:32 PM Xiang Xiao <xi...@gmail.com> wrote:
> We need to try some ioctl with read/write some driver(e.g. serial driver
> baud) internal state.
>
>
> On Fri, Apr 1, 2022 at 10:05 PM Petro Karashchenko <
> petro.karashchenko@gmail.com> wrote:
>
> > Hello,
> >
> > Here I'm talking not about driver registration permission, but more about
> > the "oflag" parameter to "open()" call.
> >
> > I just tried a quick example on MAC
> >
> > #include <fcntl.h>
> > #include <stdio.h>
> >
> > int main(void)
> > {
> > int fd = open("test.txt", 0);
> > if (fd < 0)
> > printf("A\n");
> > else
> > printf("B\n");
> >
> > return 0;
> > }
> >
> > The "B" is printed if the file exists. If you know the system that will
> run
> > this sample code and will print "A", please let me know.
> >
> > Best regards,
> > Petro
> >
> > пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <ac...@gmail.com>
> > пише:
> >
> > > I think the device file shouldn't be created with permission 000.
> > >
> > > Look inside your Linux /dev all device files have RW permission for
> > > root, some give only R for group and others.
> > >
> > > So, probably we need to fix the device register creation, not removing
> > > the flag check.
> > >
> > > BR,
> > >
> > > Alan
> > >
> > > On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> > > > It's better to check ioctl callback too since ioctl means the driver
> > has
> > > > the compatibility of read(i)and write(o).
> > > >
> > > > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> > > > petro.karashchenko@gmail.com> wrote:
> > > >
> > > >> So Alan do you suggest to remove inode_checkflags?
> > > >>
> > > >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis <
> > acassis@gmail.com>
> > > >> wrote:
> > > >>
> > > >> > Hi Petro,
> > > >> >
> > > >> > I saw your PR #1117 but I think opening a device file with flag 0
> is
> > > >> > not correct, please see the open man-pages:
> > > >> >
> > > >> > alan@dev:/tmp$ man 2 open
> > > >> >
> > > >> > The argument flags must include one of the following
> access
> > > >> > modes: O_RDONLY, O_WRONLY, or
> > > >> > O_RDWR. These request opening the file read-only,
> > write-only,
> > > >> > or read/write, respectively.
> > > >> >
> > > >> > Also the opengroup say something similar:
> > > >> >
> > > >> >
> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> > > >> >
> > > >> > "Values for oflag are constructed by a bitwise-inclusive OR of
> flags
> > > >> > from the following list, defined in <fcntl.h>. Applications shall
> > > >> > specify exactly one of the first five values (file access modes)
> > below
> > > >> > in the value of oflag:"
> > > >> >
> > > >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
> > according
> > > >> > to RFC2119 they are equivalents:
> > > >> >
> > > >> > https://www.ietf.org/rfc/rfc2119.txt
> > > >> >
> > > >> > BR,
> > > >> >
> > > >> > Alan
> > > >> >
> > > >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com>
> wrote:
> > > >> > > Hi,
> > > >> > >
> > > >> > > I want to resume this thread again because after reexamined code
> > > >> > carefully
> > > >> > > I found that VFS layer has an API
> > > >> > >
> > > >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> > > >> > > {
> > > >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> > > >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> > > >> > > {
> > > >> > > return -EACCES;
> > > >> > > }
> > > >> > > else
> > > >> > > {
> > > >> > > return OK;
> > > >> > > }
> > > >> > > }
> > > >> > >
> > > >> > > That checks if read and write handlers are available, so all our
> > > >> > discussion
> > > >> > > about R/W mode for IOCTL does not make any sense. We either need
> > to
> > > >> > remove
> > > >> > > this check or register VFS nodes with proper permissions and
> open
> > > >> > > files
> > > >> > > with correct flags. So if the driver does not have neither read
> > nor
> > > >> write
> > > >> > > handlers the "0000" mode should be used and "0" should be used
> > > during
> > > >> > > opening of a file. Or we need to remove "inode_checkflags()".
> > > >> > >
> > > >> > > Best regards,
> > > >> > > Petro
> > > >> > >
> > > >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> > > >> > > <pe...@gmail.com>
> > > >> > > пише:
> > > >> > >
> > > >> > >> I see. Thank you for the feedback. I will rework changes to get
> > > back
> > > >> > >> read permissions.
> > > >> > >>
> > > >> > >> Best regards,
> > > >> > >> Petro
> > > >> > >>
> > > >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> > > >> > >> <acassis@gmail.com
> > > >> >
> > > >> > >> пише:
> > > >> > >> >
> > > >> > >> > Hi Petro,
> > > >> > >> >
> > > >> > >> > The read permission is needed even when you just want to
> open a
> > > >> file:
> > > >> > >> >
> > > >> > >> > $ vim noreadfile
> > > >> > >> >
> > > >> > >> > $ chmod 0000 noreadfile
> > > >> > >> >
> > > >> > >> > $ ls -l noreadfile
> > > >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> > > >> > >> >
> > > >> > >> > $ cat noreadfile
> > > >> > >> > cat: noreadfile: Permission denied
> > > >> > >> >
> > > >> > >> > You can even try to create a C program just to open it, and
> it
> > > >> > >> > will
> > > >> > >> > fail.
> > > >> > >> >
> > > >> > >> > See the man page of open function:
> > > >> > >> >
> > > >> > >> > The argument flags *must* include one of the
> following
> > > >> access
> > > >> > >> > modes: O_RDONLY, O_WRONLY, or
> > > >> > >> > O_RDWR. These request opening the file read-only,
> > > >> write-only,
> > > >> > >> > or read/write, respectively.
> > > >> > >> >
> > > >> > >> > This man page makes it clear you must include an access mode,
> > but
> > > >> > >> > I
> > > >> > >> > passed 0 to the access mode flag of open() and it was
> accepted,
> > > >> > >> > but
> > > >> > >> > when the file has permission 0000 it returns -EPERM: "Failed
> to
> > > >> > >> > open
> > > >> > >> > file: error -1"
> > > >> > >> >
> > > >> > >> > BR,
> > > >> > >> >
> > > >> > >> > Alan
> > > >> > >> >
> > > >> > >> > On 1/28/22, Petro Karashchenko <petro.karashchenko@gmail.com
> >
> > > >> wrote:
> > > >> > >> > > Hello,
> > > >> > >> > >
> > > >> > >> > > Yes, but how does this relate to "0000" mode for
> > > >> > "register_driver()"?
> > > >> > >> > > Maybe you can describe some use case so it will become more
> > > >> > >> > > clear?
> > > >> > >> > > Currently ioctl works fine if driver is registered with
> > "0000"
> > > >> > >> permission
> > > >> > >> > > mode.
> > > >> > >> > >
> > > >> > >> > > Best regards,
> > > >> > >> > > Petro
> > > >> > >> > >
> > > >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> > > >> > >> > > <xiaoxiang781216@gmail.com
> > > >> >
> > > >> > >> пише:
> > > >> > >> > >>
> > > >> > >> > >> If we want to do the correct permission check, the ioctl
> > > >> > >> > >> handler
> > > >> > >> needs to
> > > >> > >> > >> check R/W bit by itself based on how the ioctl is
> > implemented.
> > > >> > >> > >> Or follow up how Linux encode the needed permission into
> > each
> > > >> > IOCTL:
> > > >> > >> > >>
> > > >> > >>
> > > >> >
> > > >>
> > >
> >
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > > >> > >> > >> and let's VFS layer do the check for each driver.
> > > >> > >> > >>
> > > >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > > >> > >> > >> petro.karashchenko@gmail.com> wrote:
> > > >> > >> > >>
> > > >> > >> > >> > Hello team,
> > > >> > >> > >> >
> > > >> > >> > >> > Recently I have noticed that there are many places in
> code
> > > >> where
> > > >> > >> > >> > register_driver() is called with non-zero mode with file
> > > >> > operation
> > > >> > >> > >> > structures that have neither read nor write APIs
> > > implemented.
> > > >> For
> > > >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
> > > >> > >> > >> > dev);"
> > > >> > >> > >> > while
> > > >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> > > >> "opamp_ioctl"
> > > >> > >> > >> > implemented. I made a PR to fix it
> > > >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
> > > >> > >> > >> > change
> > > >> > >> > >> > mode
> > > >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees
> any
> > > >> > drawback
> > > >> > >> in
> > > >> > >> > >> > such an approach? Maybe I'm missing something?
> > > >> > >> > >> >
> > > >> > >> > >> > Best regards,
> > > >> > >> > >> > Petro
> > > >> > >> > >> >
> > > >> > >> > >
> > > >> > >>
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>
Re: register_driver with 0000 mode
Posted by Xiang Xiao <xi...@gmail.com>.
We need to try some ioctl with read/write some driver(e.g. serial driver
baud) internal state.
On Fri, Apr 1, 2022 at 10:05 PM Petro Karashchenko <
petro.karashchenko@gmail.com> wrote:
> Hello,
>
> Here I'm talking not about driver registration permission, but more about
> the "oflag" parameter to "open()" call.
>
> I just tried a quick example on MAC
>
> #include <fcntl.h>
> #include <stdio.h>
>
> int main(void)
> {
> int fd = open("test.txt", 0);
> if (fd < 0)
> printf("A\n");
> else
> printf("B\n");
>
> return 0;
> }
>
> The "B" is printed if the file exists. If you know the system that will run
> this sample code and will print "A", please let me know.
>
> Best regards,
> Petro
>
> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <ac...@gmail.com>
> пише:
>
> > I think the device file shouldn't be created with permission 000.
> >
> > Look inside your Linux /dev all device files have RW permission for
> > root, some give only R for group and others.
> >
> > So, probably we need to fix the device register creation, not removing
> > the flag check.
> >
> > BR,
> >
> > Alan
> >
> > On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> > > It's better to check ioctl callback too since ioctl means the driver
> has
> > > the compatibility of read(i)and write(o).
> > >
> > > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> > > petro.karashchenko@gmail.com> wrote:
> > >
> > >> So Alan do you suggest to remove inode_checkflags?
> > >>
> > >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis <
> acassis@gmail.com>
> > >> wrote:
> > >>
> > >> > Hi Petro,
> > >> >
> > >> > I saw your PR #1117 but I think opening a device file with flag 0 is
> > >> > not correct, please see the open man-pages:
> > >> >
> > >> > alan@dev:/tmp$ man 2 open
> > >> >
> > >> > The argument flags must include one of the following access
> > >> > modes: O_RDONLY, O_WRONLY, or
> > >> > O_RDWR. These request opening the file read-only,
> write-only,
> > >> > or read/write, respectively.
> > >> >
> > >> > Also the opengroup say something similar:
> > >> >
> > >> >
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> > >> >
> > >> > "Values for oflag are constructed by a bitwise-inclusive OR of flags
> > >> > from the following list, defined in <fcntl.h>. Applications shall
> > >> > specify exactly one of the first five values (file access modes)
> below
> > >> > in the value of oflag:"
> > >> >
> > >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but
> according
> > >> > to RFC2119 they are equivalents:
> > >> >
> > >> > https://www.ietf.org/rfc/rfc2119.txt
> > >> >
> > >> > BR,
> > >> >
> > >> > Alan
> > >> >
> > >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > >> > > Hi,
> > >> > >
> > >> > > I want to resume this thread again because after reexamined code
> > >> > carefully
> > >> > > I found that VFS layer has an API
> > >> > >
> > >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> > >> > > {
> > >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> > >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> > >> > > {
> > >> > > return -EACCES;
> > >> > > }
> > >> > > else
> > >> > > {
> > >> > > return OK;
> > >> > > }
> > >> > > }
> > >> > >
> > >> > > That checks if read and write handlers are available, so all our
> > >> > discussion
> > >> > > about R/W mode for IOCTL does not make any sense. We either need
> to
> > >> > remove
> > >> > > this check or register VFS nodes with proper permissions and open
> > >> > > files
> > >> > > with correct flags. So if the driver does not have neither read
> nor
> > >> write
> > >> > > handlers the "0000" mode should be used and "0" should be used
> > during
> > >> > > opening of a file. Or we need to remove "inode_checkflags()".
> > >> > >
> > >> > > Best regards,
> > >> > > Petro
> > >> > >
> > >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> > >> > > <pe...@gmail.com>
> > >> > > пише:
> > >> > >
> > >> > >> I see. Thank you for the feedback. I will rework changes to get
> > back
> > >> > >> read permissions.
> > >> > >>
> > >> > >> Best regards,
> > >> > >> Petro
> > >> > >>
> > >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> > >> > >> <acassis@gmail.com
> > >> >
> > >> > >> пише:
> > >> > >> >
> > >> > >> > Hi Petro,
> > >> > >> >
> > >> > >> > The read permission is needed even when you just want to open a
> > >> file:
> > >> > >> >
> > >> > >> > $ vim noreadfile
> > >> > >> >
> > >> > >> > $ chmod 0000 noreadfile
> > >> > >> >
> > >> > >> > $ ls -l noreadfile
> > >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> > >> > >> >
> > >> > >> > $ cat noreadfile
> > >> > >> > cat: noreadfile: Permission denied
> > >> > >> >
> > >> > >> > You can even try to create a C program just to open it, and it
> > >> > >> > will
> > >> > >> > fail.
> > >> > >> >
> > >> > >> > See the man page of open function:
> > >> > >> >
> > >> > >> > The argument flags *must* include one of the following
> > >> access
> > >> > >> > modes: O_RDONLY, O_WRONLY, or
> > >> > >> > O_RDWR. These request opening the file read-only,
> > >> write-only,
> > >> > >> > or read/write, respectively.
> > >> > >> >
> > >> > >> > This man page makes it clear you must include an access mode,
> but
> > >> > >> > I
> > >> > >> > passed 0 to the access mode flag of open() and it was accepted,
> > >> > >> > but
> > >> > >> > when the file has permission 0000 it returns -EPERM: "Failed to
> > >> > >> > open
> > >> > >> > file: error -1"
> > >> > >> >
> > >> > >> > BR,
> > >> > >> >
> > >> > >> > Alan
> > >> > >> >
> > >> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
> > >> wrote:
> > >> > >> > > Hello,
> > >> > >> > >
> > >> > >> > > Yes, but how does this relate to "0000" mode for
> > >> > "register_driver()"?
> > >> > >> > > Maybe you can describe some use case so it will become more
> > >> > >> > > clear?
> > >> > >> > > Currently ioctl works fine if driver is registered with
> "0000"
> > >> > >> permission
> > >> > >> > > mode.
> > >> > >> > >
> > >> > >> > > Best regards,
> > >> > >> > > Petro
> > >> > >> > >
> > >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> > >> > >> > > <xiaoxiang781216@gmail.com
> > >> >
> > >> > >> пише:
> > >> > >> > >>
> > >> > >> > >> If we want to do the correct permission check, the ioctl
> > >> > >> > >> handler
> > >> > >> needs to
> > >> > >> > >> check R/W bit by itself based on how the ioctl is
> implemented.
> > >> > >> > >> Or follow up how Linux encode the needed permission into
> each
> > >> > IOCTL:
> > >> > >> > >>
> > >> > >>
> > >> >
> > >>
> >
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > >> > >> > >> and let's VFS layer do the check for each driver.
> > >> > >> > >>
> > >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > >> > >> > >> petro.karashchenko@gmail.com> wrote:
> > >> > >> > >>
> > >> > >> > >> > Hello team,
> > >> > >> > >> >
> > >> > >> > >> > Recently I have noticed that there are many places in code
> > >> where
> > >> > >> > >> > register_driver() is called with non-zero mode with file
> > >> > operation
> > >> > >> > >> > structures that have neither read nor write APIs
> > implemented.
> > >> For
> > >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
> > >> > >> > >> > dev);"
> > >> > >> > >> > while
> > >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> > >> "opamp_ioctl"
> > >> > >> > >> > implemented. I made a PR to fix it
> > >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
> > >> > >> > >> > change
> > >> > >> > >> > mode
> > >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
> > >> > drawback
> > >> > >> in
> > >> > >> > >> > such an approach? Maybe I'm missing something?
> > >> > >> > >> >
> > >> > >> > >> > Best regards,
> > >> > >> > >> > Petro
> > >> > >> > >> >
> > >> > >> > >
> > >> > >>
> > >> > >
> > >> >
> > >>
> > >
> >
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Hello,
Here I'm talking not about driver registration permission, but more about
the "oflag" parameter to "open()" call.
I just tried a quick example on MAC
#include <fcntl.h>
#include <stdio.h>
int main(void)
{
int fd = open("test.txt", 0);
if (fd < 0)
printf("A\n");
else
printf("B\n");
return 0;
}
The "B" is printed if the file exists. If you know the system that will run
this sample code and will print "A", please let me know.
Best regards,
Petro
пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <ac...@gmail.com> пише:
> I think the device file shouldn't be created with permission 000.
>
> Look inside your Linux /dev all device files have RW permission for
> root, some give only R for group and others.
>
> So, probably we need to fix the device register creation, not removing
> the flag check.
>
> BR,
>
> Alan
>
> On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> > It's better to check ioctl callback too since ioctl means the driver has
> > the compatibility of read(i)and write(o).
> >
> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> > petro.karashchenko@gmail.com> wrote:
> >
> >> So Alan do you suggest to remove inode_checkflags?
> >>
> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis <ac...@gmail.com>
> >> wrote:
> >>
> >> > Hi Petro,
> >> >
> >> > I saw your PR #1117 but I think opening a device file with flag 0 is
> >> > not correct, please see the open man-pages:
> >> >
> >> > alan@dev:/tmp$ man 2 open
> >> >
> >> > The argument flags must include one of the following access
> >> > modes: O_RDONLY, O_WRONLY, or
> >> > O_RDWR. These request opening the file read-only, write-only,
> >> > or read/write, respectively.
> >> >
> >> > Also the opengroup say something similar:
> >> >
> >> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> >> >
> >> > "Values for oflag are constructed by a bitwise-inclusive OR of flags
> >> > from the following list, defined in <fcntl.h>. Applications shall
> >> > specify exactly one of the first five values (file access modes) below
> >> > in the value of oflag:"
> >> >
> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but according
> >> > to RFC2119 they are equivalents:
> >> >
> >> > https://www.ietf.org/rfc/rfc2119.txt
> >> >
> >> > BR,
> >> >
> >> > Alan
> >> >
> >> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> >> > > Hi,
> >> > >
> >> > > I want to resume this thread again because after reexamined code
> >> > carefully
> >> > > I found that VFS layer has an API
> >> > >
> >> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> >> > > {
> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> >> > > {
> >> > > return -EACCES;
> >> > > }
> >> > > else
> >> > > {
> >> > > return OK;
> >> > > }
> >> > > }
> >> > >
> >> > > That checks if read and write handlers are available, so all our
> >> > discussion
> >> > > about R/W mode for IOCTL does not make any sense. We either need to
> >> > remove
> >> > > this check or register VFS nodes with proper permissions and open
> >> > > files
> >> > > with correct flags. So if the driver does not have neither read nor
> >> write
> >> > > handlers the "0000" mode should be used and "0" should be used
> during
> >> > > opening of a file. Or we need to remove "inode_checkflags()".
> >> > >
> >> > > Best regards,
> >> > > Petro
> >> > >
> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> >> > > <pe...@gmail.com>
> >> > > пише:
> >> > >
> >> > >> I see. Thank you for the feedback. I will rework changes to get
> back
> >> > >> read permissions.
> >> > >>
> >> > >> Best regards,
> >> > >> Petro
> >> > >>
> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
> >> > >> <acassis@gmail.com
> >> >
> >> > >> пише:
> >> > >> >
> >> > >> > Hi Petro,
> >> > >> >
> >> > >> > The read permission is needed even when you just want to open a
> >> file:
> >> > >> >
> >> > >> > $ vim noreadfile
> >> > >> >
> >> > >> > $ chmod 0000 noreadfile
> >> > >> >
> >> > >> > $ ls -l noreadfile
> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> >> > >> >
> >> > >> > $ cat noreadfile
> >> > >> > cat: noreadfile: Permission denied
> >> > >> >
> >> > >> > You can even try to create a C program just to open it, and it
> >> > >> > will
> >> > >> > fail.
> >> > >> >
> >> > >> > See the man page of open function:
> >> > >> >
> >> > >> > The argument flags *must* include one of the following
> >> access
> >> > >> > modes: O_RDONLY, O_WRONLY, or
> >> > >> > O_RDWR. These request opening the file read-only,
> >> write-only,
> >> > >> > or read/write, respectively.
> >> > >> >
> >> > >> > This man page makes it clear you must include an access mode, but
> >> > >> > I
> >> > >> > passed 0 to the access mode flag of open() and it was accepted,
> >> > >> > but
> >> > >> > when the file has permission 0000 it returns -EPERM: "Failed to
> >> > >> > open
> >> > >> > file: error -1"
> >> > >> >
> >> > >> > BR,
> >> > >> >
> >> > >> > Alan
> >> > >> >
> >> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
> >> wrote:
> >> > >> > > Hello,
> >> > >> > >
> >> > >> > > Yes, but how does this relate to "0000" mode for
> >> > "register_driver()"?
> >> > >> > > Maybe you can describe some use case so it will become more
> >> > >> > > clear?
> >> > >> > > Currently ioctl works fine if driver is registered with "0000"
> >> > >> permission
> >> > >> > > mode.
> >> > >> > >
> >> > >> > > Best regards,
> >> > >> > > Petro
> >> > >> > >
> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
> >> > >> > > <xiaoxiang781216@gmail.com
> >> >
> >> > >> пише:
> >> > >> > >>
> >> > >> > >> If we want to do the correct permission check, the ioctl
> >> > >> > >> handler
> >> > >> needs to
> >> > >> > >> check R/W bit by itself based on how the ioctl is implemented.
> >> > >> > >> Or follow up how Linux encode the needed permission into each
> >> > IOCTL:
> >> > >> > >>
> >> > >>
> >> >
> >>
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> >> > >> > >> and let's VFS layer do the check for each driver.
> >> > >> > >>
> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> >> > >> > >> petro.karashchenko@gmail.com> wrote:
> >> > >> > >>
> >> > >> > >> > Hello team,
> >> > >> > >> >
> >> > >> > >> > Recently I have noticed that there are many places in code
> >> where
> >> > >> > >> > register_driver() is called with non-zero mode with file
> >> > operation
> >> > >> > >> > structures that have neither read nor write APIs
> implemented.
> >> For
> >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
> >> > >> > >> > dev);"
> >> > >> > >> > while
> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> >> "opamp_ioctl"
> >> > >> > >> > implemented. I made a PR to fix it
> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
> >> > >> > >> > change
> >> > >> > >> > mode
> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
> >> > drawback
> >> > >> in
> >> > >> > >> > such an approach? Maybe I'm missing something?
> >> > >> > >> >
> >> > >> > >> > Best regards,
> >> > >> > >> > Petro
> >> > >> > >> >
> >> > >> > >
> >> > >>
> >> > >
> >> >
> >>
> >
>
Re: register_driver with 0000 mode
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
I think the device file shouldn't be created with permission 000.
Look inside your Linux /dev all device files have RW permission for
root, some give only R for group and others.
So, probably we need to fix the device register creation, not removing
the flag check.
BR,
Alan
On 4/1/22, Xiang Xiao <xi...@gmail.com> wrote:
> It's better to check ioctl callback too since ioctl means the driver has
> the compatibility of read(i)and write(o).
>
> On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
> petro.karashchenko@gmail.com> wrote:
>
>> So Alan do you suggest to remove inode_checkflags?
>>
>> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis <ac...@gmail.com>
>> wrote:
>>
>> > Hi Petro,
>> >
>> > I saw your PR #1117 but I think opening a device file with flag 0 is
>> > not correct, please see the open man-pages:
>> >
>> > alan@dev:/tmp$ man 2 open
>> >
>> > The argument flags must include one of the following access
>> > modes: O_RDONLY, O_WRONLY, or
>> > O_RDWR. These request opening the file read-only, write-only,
>> > or read/write, respectively.
>> >
>> > Also the opengroup say something similar:
>> >
>> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
>> >
>> > "Values for oflag are constructed by a bitwise-inclusive OR of flags
>> > from the following list, defined in <fcntl.h>. Applications shall
>> > specify exactly one of the first five values (file access modes) below
>> > in the value of oflag:"
>> >
>> > The man pages uses "MUST", the OpenGroups uses "SHALL", but according
>> > to RFC2119 they are equivalents:
>> >
>> > https://www.ietf.org/rfc/rfc2119.txt
>> >
>> > BR,
>> >
>> > Alan
>> >
>> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
>> > > Hi,
>> > >
>> > > I want to resume this thread again because after reexamined code
>> > carefully
>> > > I found that VFS layer has an API
>> > >
>> > > int inode_checkflags(FAR struct inode *inode, int oflags)
>> > > {
>> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
>> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
>> > > {
>> > > return -EACCES;
>> > > }
>> > > else
>> > > {
>> > > return OK;
>> > > }
>> > > }
>> > >
>> > > That checks if read and write handlers are available, so all our
>> > discussion
>> > > about R/W mode for IOCTL does not make any sense. We either need to
>> > remove
>> > > this check or register VFS nodes with proper permissions and open
>> > > files
>> > > with correct flags. So if the driver does not have neither read nor
>> write
>> > > handlers the "0000" mode should be used and "0" should be used during
>> > > opening of a file. Or we need to remove "inode_checkflags()".
>> > >
>> > > Best regards,
>> > > Petro
>> > >
>> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
>> > > <pe...@gmail.com>
>> > > пише:
>> > >
>> > >> I see. Thank you for the feedback. I will rework changes to get back
>> > >> read permissions.
>> > >>
>> > >> Best regards,
>> > >> Petro
>> > >>
>> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis
>> > >> <acassis@gmail.com
>> >
>> > >> пише:
>> > >> >
>> > >> > Hi Petro,
>> > >> >
>> > >> > The read permission is needed even when you just want to open a
>> file:
>> > >> >
>> > >> > $ vim noreadfile
>> > >> >
>> > >> > $ chmod 0000 noreadfile
>> > >> >
>> > >> > $ ls -l noreadfile
>> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
>> > >> >
>> > >> > $ cat noreadfile
>> > >> > cat: noreadfile: Permission denied
>> > >> >
>> > >> > You can even try to create a C program just to open it, and it
>> > >> > will
>> > >> > fail.
>> > >> >
>> > >> > See the man page of open function:
>> > >> >
>> > >> > The argument flags *must* include one of the following
>> access
>> > >> > modes: O_RDONLY, O_WRONLY, or
>> > >> > O_RDWR. These request opening the file read-only,
>> write-only,
>> > >> > or read/write, respectively.
>> > >> >
>> > >> > This man page makes it clear you must include an access mode, but
>> > >> > I
>> > >> > passed 0 to the access mode flag of open() and it was accepted,
>> > >> > but
>> > >> > when the file has permission 0000 it returns -EPERM: "Failed to
>> > >> > open
>> > >> > file: error -1"
>> > >> >
>> > >> > BR,
>> > >> >
>> > >> > Alan
>> > >> >
>> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
>> wrote:
>> > >> > > Hello,
>> > >> > >
>> > >> > > Yes, but how does this relate to "0000" mode for
>> > "register_driver()"?
>> > >> > > Maybe you can describe some use case so it will become more
>> > >> > > clear?
>> > >> > > Currently ioctl works fine if driver is registered with "0000"
>> > >> permission
>> > >> > > mode.
>> > >> > >
>> > >> > > Best regards,
>> > >> > > Petro
>> > >> > >
>> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao
>> > >> > > <xiaoxiang781216@gmail.com
>> >
>> > >> пише:
>> > >> > >>
>> > >> > >> If we want to do the correct permission check, the ioctl
>> > >> > >> handler
>> > >> needs to
>> > >> > >> check R/W bit by itself based on how the ioctl is implemented.
>> > >> > >> Or follow up how Linux encode the needed permission into each
>> > IOCTL:
>> > >> > >>
>> > >>
>> >
>> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
>> > >> > >> and let's VFS layer do the check for each driver.
>> > >> > >>
>> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
>> > >> > >> petro.karashchenko@gmail.com> wrote:
>> > >> > >>
>> > >> > >> > Hello team,
>> > >> > >> >
>> > >> > >> > Recently I have noticed that there are many places in code
>> where
>> > >> > >> > register_driver() is called with non-zero mode with file
>> > operation
>> > >> > >> > structures that have neither read nor write APIs implemented.
>> For
>> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444,
>> > >> > >> > dev);"
>> > >> > >> > while
>> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
>> "opamp_ioctl"
>> > >> > >> > implemented. I made a PR to fix it
>> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and
>> > >> > >> > change
>> > >> > >> > mode
>> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
>> > drawback
>> > >> in
>> > >> > >> > such an approach? Maybe I'm missing something?
>> > >> > >> >
>> > >> > >> > Best regards,
>> > >> > >> > Petro
>> > >> > >> >
>> > >> > >
>> > >>
>> > >
>> >
>>
>
Re: register_driver with 0000 mode
Posted by Xiang Xiao <xi...@gmail.com>.
It's better to check ioctl callback too since ioctl means the driver has
the compatibility of read(i)and write(o).
On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko <
petro.karashchenko@gmail.com> wrote:
> So Alan do you suggest to remove inode_checkflags?
>
> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis <ac...@gmail.com>
> wrote:
>
> > Hi Petro,
> >
> > I saw your PR #1117 but I think opening a device file with flag 0 is
> > not correct, please see the open man-pages:
> >
> > alan@dev:/tmp$ man 2 open
> >
> > The argument flags must include one of the following access
> > modes: O_RDONLY, O_WRONLY, or
> > O_RDWR. These request opening the file read-only, write-only,
> > or read/write, respectively.
> >
> > Also the opengroup say something similar:
> >
> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> >
> > "Values for oflag are constructed by a bitwise-inclusive OR of flags
> > from the following list, defined in <fcntl.h>. Applications shall
> > specify exactly one of the first five values (file access modes) below
> > in the value of oflag:"
> >
> > The man pages uses "MUST", the OpenGroups uses "SHALL", but according
> > to RFC2119 they are equivalents:
> >
> > https://www.ietf.org/rfc/rfc2119.txt
> >
> > BR,
> >
> > Alan
> >
> > On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > > Hi,
> > >
> > > I want to resume this thread again because after reexamined code
> > carefully
> > > I found that VFS layer has an API
> > >
> > > int inode_checkflags(FAR struct inode *inode, int oflags)
> > > {
> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> > > {
> > > return -EACCES;
> > > }
> > > else
> > > {
> > > return OK;
> > > }
> > > }
> > >
> > > That checks if read and write handlers are available, so all our
> > discussion
> > > about R/W mode for IOCTL does not make any sense. We either need to
> > remove
> > > this check or register VFS nodes with proper permissions and open files
> > > with correct flags. So if the driver does not have neither read nor
> write
> > > handlers the "0000" mode should be used and "0" should be used during
> > > opening of a file. Or we need to remove "inode_checkflags()".
> > >
> > > Best regards,
> > > Petro
> > >
> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> > > <pe...@gmail.com>
> > > пише:
> > >
> > >> I see. Thank you for the feedback. I will rework changes to get back
> > >> read permissions.
> > >>
> > >> Best regards,
> > >> Petro
> > >>
> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis <acassis@gmail.com
> >
> > >> пише:
> > >> >
> > >> > Hi Petro,
> > >> >
> > >> > The read permission is needed even when you just want to open a
> file:
> > >> >
> > >> > $ vim noreadfile
> > >> >
> > >> > $ chmod 0000 noreadfile
> > >> >
> > >> > $ ls -l noreadfile
> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> > >> >
> > >> > $ cat noreadfile
> > >> > cat: noreadfile: Permission denied
> > >> >
> > >> > You can even try to create a C program just to open it, and it will
> > >> > fail.
> > >> >
> > >> > See the man page of open function:
> > >> >
> > >> > The argument flags *must* include one of the following
> access
> > >> > modes: O_RDONLY, O_WRONLY, or
> > >> > O_RDWR. These request opening the file read-only,
> write-only,
> > >> > or read/write, respectively.
> > >> >
> > >> > This man page makes it clear you must include an access mode, but I
> > >> > passed 0 to the access mode flag of open() and it was accepted, but
> > >> > when the file has permission 0000 it returns -EPERM: "Failed to open
> > >> > file: error -1"
> > >> >
> > >> > BR,
> > >> >
> > >> > Alan
> > >> >
> > >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com>
> wrote:
> > >> > > Hello,
> > >> > >
> > >> > > Yes, but how does this relate to "0000" mode for
> > "register_driver()"?
> > >> > > Maybe you can describe some use case so it will become more clear?
> > >> > > Currently ioctl works fine if driver is registered with "0000"
> > >> permission
> > >> > > mode.
> > >> > >
> > >> > > Best regards,
> > >> > > Petro
> > >> > >
> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xiaoxiang781216@gmail.com
> >
> > >> пише:
> > >> > >>
> > >> > >> If we want to do the correct permission check, the ioctl handler
> > >> needs to
> > >> > >> check R/W bit by itself based on how the ioctl is implemented.
> > >> > >> Or follow up how Linux encode the needed permission into each
> > IOCTL:
> > >> > >>
> > >>
> >
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > >> > >> and let's VFS layer do the check for each driver.
> > >> > >>
> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > >> > >> petro.karashchenko@gmail.com> wrote:
> > >> > >>
> > >> > >> > Hello team,
> > >> > >> >
> > >> > >> > Recently I have noticed that there are many places in code
> where
> > >> > >> > register_driver() is called with non-zero mode with file
> > operation
> > >> > >> > structures that have neither read nor write APIs implemented.
> For
> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444, dev);"
> > >> > >> > while
> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and
> "opamp_ioctl"
> > >> > >> > implemented. I made a PR to fix it
> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and change
> > >> > >> > mode
> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any
> > drawback
> > >> in
> > >> > >> > such an approach? Maybe I'm missing something?
> > >> > >> >
> > >> > >> > Best regards,
> > >> > >> > Petro
> > >> > >> >
> > >> > >
> > >>
> > >
> >
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
So Alan do you suggest to remove inode_checkflags?
On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis <ac...@gmail.com>
wrote:
> Hi Petro,
>
> I saw your PR #1117 but I think opening a device file with flag 0 is
> not correct, please see the open man-pages:
>
> alan@dev:/tmp$ man 2 open
>
> The argument flags must include one of the following access
> modes: O_RDONLY, O_WRONLY, or
> O_RDWR. These request opening the file read-only, write-only,
> or read/write, respectively.
>
> Also the opengroup say something similar:
>
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
>
> "Values for oflag are constructed by a bitwise-inclusive OR of flags
> from the following list, defined in <fcntl.h>. Applications shall
> specify exactly one of the first five values (file access modes) below
> in the value of oflag:"
>
> The man pages uses "MUST", the OpenGroups uses "SHALL", but according
> to RFC2119 they are equivalents:
>
> https://www.ietf.org/rfc/rfc2119.txt
>
> BR,
>
> Alan
>
> On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > Hi,
> >
> > I want to resume this thread again because after reexamined code
> carefully
> > I found that VFS layer has an API
> >
> > int inode_checkflags(FAR struct inode *inode, int oflags)
> > {
> > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> > {
> > return -EACCES;
> > }
> > else
> > {
> > return OK;
> > }
> > }
> >
> > That checks if read and write handlers are available, so all our
> discussion
> > about R/W mode for IOCTL does not make any sense. We either need to
> remove
> > this check or register VFS nodes with proper permissions and open files
> > with correct flags. So if the driver does not have neither read nor write
> > handlers the "0000" mode should be used and "0" should be used during
> > opening of a file. Or we need to remove "inode_checkflags()".
> >
> > Best regards,
> > Petro
> >
> > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> > <pe...@gmail.com>
> > пише:
> >
> >> I see. Thank you for the feedback. I will rework changes to get back
> >> read permissions.
> >>
> >> Best regards,
> >> Petro
> >>
> >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis <ac...@gmail.com>
> >> пише:
> >> >
> >> > Hi Petro,
> >> >
> >> > The read permission is needed even when you just want to open a file:
> >> >
> >> > $ vim noreadfile
> >> >
> >> > $ chmod 0000 noreadfile
> >> >
> >> > $ ls -l noreadfile
> >> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> >> >
> >> > $ cat noreadfile
> >> > cat: noreadfile: Permission denied
> >> >
> >> > You can even try to create a C program just to open it, and it will
> >> > fail.
> >> >
> >> > See the man page of open function:
> >> >
> >> > The argument flags *must* include one of the following access
> >> > modes: O_RDONLY, O_WRONLY, or
> >> > O_RDWR. These request opening the file read-only, write-only,
> >> > or read/write, respectively.
> >> >
> >> > This man page makes it clear you must include an access mode, but I
> >> > passed 0 to the access mode flag of open() and it was accepted, but
> >> > when the file has permission 0000 it returns -EPERM: "Failed to open
> >> > file: error -1"
> >> >
> >> > BR,
> >> >
> >> > Alan
> >> >
> >> > On 1/28/22, Petro Karashchenko <pe...@gmail.com> wrote:
> >> > > Hello,
> >> > >
> >> > > Yes, but how does this relate to "0000" mode for
> "register_driver()"?
> >> > > Maybe you can describe some use case so it will become more clear?
> >> > > Currently ioctl works fine if driver is registered with "0000"
> >> permission
> >> > > mode.
> >> > >
> >> > > Best regards,
> >> > > Petro
> >> > >
> >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xi...@gmail.com>
> >> пише:
> >> > >>
> >> > >> If we want to do the correct permission check, the ioctl handler
> >> needs to
> >> > >> check R/W bit by itself based on how the ioctl is implemented.
> >> > >> Or follow up how Linux encode the needed permission into each
> IOCTL:
> >> > >>
> >>
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> >> > >> and let's VFS layer do the check for each driver.
> >> > >>
> >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> >> > >> petro.karashchenko@gmail.com> wrote:
> >> > >>
> >> > >> > Hello team,
> >> > >> >
> >> > >> > Recently I have noticed that there are many places in code where
> >> > >> > register_driver() is called with non-zero mode with file
> operation
> >> > >> > structures that have neither read nor write APIs implemented. For
> >> > >> > example "ret = register_driver(path, &opamp_fops, 0444, dev);"
> >> > >> > while
> >> > >> > opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
> >> > >> > implemented. I made a PR to fix it
> >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and change
> >> > >> > mode
> >> > >> > from "0444" to "0000", but want to ask if anyone sees any
> drawback
> >> in
> >> > >> > such an approach? Maybe I'm missing something?
> >> > >> >
> >> > >> > Best regards,
> >> > >> > Petro
> >> > >> >
> >> > >
> >>
> >
>
Re: register_driver with 0000 mode
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
Hi Petro,
I saw your PR #1117 but I think opening a device file with flag 0 is
not correct, please see the open man-pages:
alan@dev:/tmp$ man 2 open
The argument flags must include one of the following access
modes: O_RDONLY, O_WRONLY, or
O_RDWR. These request opening the file read-only, write-only,
or read/write, respectively.
Also the opengroup say something similar:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
"Values for oflag are constructed by a bitwise-inclusive OR of flags
from the following list, defined in <fcntl.h>. Applications shall
specify exactly one of the first five values (file access modes) below
in the value of oflag:"
The man pages uses "MUST", the OpenGroups uses "SHALL", but according
to RFC2119 they are equivalents:
https://www.ietf.org/rfc/rfc2119.txt
BR,
Alan
On 4/1/22, Petro Karashchenko <pe...@gmail.com> wrote:
> Hi,
>
> I want to resume this thread again because after reexamined code carefully
> I found that VFS layer has an API
>
> int inode_checkflags(FAR struct inode *inode, int oflags)
> {
> if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
> ((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
> {
> return -EACCES;
> }
> else
> {
> return OK;
> }
> }
>
> That checks if read and write handlers are available, so all our discussion
> about R/W mode for IOCTL does not make any sense. We either need to remove
> this check or register VFS nodes with proper permissions and open files
> with correct flags. So if the driver does not have neither read nor write
> handlers the "0000" mode should be used and "0" should be used during
> opening of a file. Or we need to remove "inode_checkflags()".
>
> Best regards,
> Petro
>
> пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko
> <pe...@gmail.com>
> пише:
>
>> I see. Thank you for the feedback. I will rework changes to get back
>> read permissions.
>>
>> Best regards,
>> Petro
>>
>> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis <ac...@gmail.com>
>> пише:
>> >
>> > Hi Petro,
>> >
>> > The read permission is needed even when you just want to open a file:
>> >
>> > $ vim noreadfile
>> >
>> > $ chmod 0000 noreadfile
>> >
>> > $ ls -l noreadfile
>> > ---------- 1 user user 5 jan 28 09:24 noreadfile
>> >
>> > $ cat noreadfile
>> > cat: noreadfile: Permission denied
>> >
>> > You can even try to create a C program just to open it, and it will
>> > fail.
>> >
>> > See the man page of open function:
>> >
>> > The argument flags *must* include one of the following access
>> > modes: O_RDONLY, O_WRONLY, or
>> > O_RDWR. These request opening the file read-only, write-only,
>> > or read/write, respectively.
>> >
>> > This man page makes it clear you must include an access mode, but I
>> > passed 0 to the access mode flag of open() and it was accepted, but
>> > when the file has permission 0000 it returns -EPERM: "Failed to open
>> > file: error -1"
>> >
>> > BR,
>> >
>> > Alan
>> >
>> > On 1/28/22, Petro Karashchenko <pe...@gmail.com> wrote:
>> > > Hello,
>> > >
>> > > Yes, but how does this relate to "0000" mode for "register_driver()"?
>> > > Maybe you can describe some use case so it will become more clear?
>> > > Currently ioctl works fine if driver is registered with "0000"
>> permission
>> > > mode.
>> > >
>> > > Best regards,
>> > > Petro
>> > >
>> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xi...@gmail.com>
>> пише:
>> > >>
>> > >> If we want to do the correct permission check, the ioctl handler
>> needs to
>> > >> check R/W bit by itself based on how the ioctl is implemented.
>> > >> Or follow up how Linux encode the needed permission into each IOCTL:
>> > >>
>> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
>> > >> and let's VFS layer do the check for each driver.
>> > >>
>> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
>> > >> petro.karashchenko@gmail.com> wrote:
>> > >>
>> > >> > Hello team,
>> > >> >
>> > >> > Recently I have noticed that there are many places in code where
>> > >> > register_driver() is called with non-zero mode with file operation
>> > >> > structures that have neither read nor write APIs implemented. For
>> > >> > example "ret = register_driver(path, &opamp_fops, 0444, dev);"
>> > >> > while
>> > >> > opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
>> > >> > implemented. I made a PR to fix it
>> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and change
>> > >> > mode
>> > >> > from "0444" to "0000", but want to ask if anyone sees any drawback
>> in
>> > >> > such an approach? Maybe I'm missing something?
>> > >> >
>> > >> > Best regards,
>> > >> > Petro
>> > >> >
>> > >
>>
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Hi,
I want to resume this thread again because after reexamined code carefully
I found that VFS layer has an API
int inode_checkflags(FAR struct inode *inode, int oflags)
{
if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) ||
((oflags & O_WROK) != 0 && !inode->u.i_ops->write))
{
return -EACCES;
}
else
{
return OK;
}
}
That checks if read and write handlers are available, so all our discussion
about R/W mode for IOCTL does not make any sense. We either need to remove
this check or register VFS nodes with proper permissions and open files
with correct flags. So if the driver does not have neither read nor write
handlers the "0000" mode should be used and "0" should be used during
opening of a file. Or we need to remove "inode_checkflags()".
Best regards,
Petro
пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko <pe...@gmail.com>
пише:
> I see. Thank you for the feedback. I will rework changes to get back
> read permissions.
>
> Best regards,
> Petro
>
> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis <ac...@gmail.com>
> пише:
> >
> > Hi Petro,
> >
> > The read permission is needed even when you just want to open a file:
> >
> > $ vim noreadfile
> >
> > $ chmod 0000 noreadfile
> >
> > $ ls -l noreadfile
> > ---------- 1 user user 5 jan 28 09:24 noreadfile
> >
> > $ cat noreadfile
> > cat: noreadfile: Permission denied
> >
> > You can even try to create a C program just to open it, and it will fail.
> >
> > See the man page of open function:
> >
> > The argument flags *must* include one of the following access
> > modes: O_RDONLY, O_WRONLY, or
> > O_RDWR. These request opening the file read-only, write-only,
> > or read/write, respectively.
> >
> > This man page makes it clear you must include an access mode, but I
> > passed 0 to the access mode flag of open() and it was accepted, but
> > when the file has permission 0000 it returns -EPERM: "Failed to open
> > file: error -1"
> >
> > BR,
> >
> > Alan
> >
> > On 1/28/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > > Hello,
> > >
> > > Yes, but how does this relate to "0000" mode for "register_driver()"?
> > > Maybe you can describe some use case so it will become more clear?
> > > Currently ioctl works fine if driver is registered with "0000"
> permission
> > > mode.
> > >
> > > Best regards,
> > > Petro
> > >
> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xi...@gmail.com>
> пише:
> > >>
> > >> If we want to do the correct permission check, the ioctl handler
> needs to
> > >> check R/W bit by itself based on how the ioctl is implemented.
> > >> Or follow up how Linux encode the needed permission into each IOCTL:
> > >>
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > >> and let's VFS layer do the check for each driver.
> > >>
> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > >> petro.karashchenko@gmail.com> wrote:
> > >>
> > >> > Hello team,
> > >> >
> > >> > Recently I have noticed that there are many places in code where
> > >> > register_driver() is called with non-zero mode with file operation
> > >> > structures that have neither read nor write APIs implemented. For
> > >> > example "ret = register_driver(path, &opamp_fops, 0444, dev);" while
> > >> > opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
> > >> > implemented. I made a PR to fix it
> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and change mode
> > >> > from "0444" to "0000", but want to ask if anyone sees any drawback
> in
> > >> > such an approach? Maybe I'm missing something?
> > >> >
> > >> > Best regards,
> > >> > Petro
> > >> >
> > >
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
I see. Thank you for the feedback. I will rework changes to get back
read permissions.
Best regards,
Petro
пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis <ac...@gmail.com> пише:
>
> Hi Petro,
>
> The read permission is needed even when you just want to open a file:
>
> $ vim noreadfile
>
> $ chmod 0000 noreadfile
>
> $ ls -l noreadfile
> ---------- 1 user user 5 jan 28 09:24 noreadfile
>
> $ cat noreadfile
> cat: noreadfile: Permission denied
>
> You can even try to create a C program just to open it, and it will fail.
>
> See the man page of open function:
>
> The argument flags *must* include one of the following access
> modes: O_RDONLY, O_WRONLY, or
> O_RDWR. These request opening the file read-only, write-only,
> or read/write, respectively.
>
> This man page makes it clear you must include an access mode, but I
> passed 0 to the access mode flag of open() and it was accepted, but
> when the file has permission 0000 it returns -EPERM: "Failed to open
> file: error -1"
>
> BR,
>
> Alan
>
> On 1/28/22, Petro Karashchenko <pe...@gmail.com> wrote:
> > Hello,
> >
> > Yes, but how does this relate to "0000" mode for "register_driver()"?
> > Maybe you can describe some use case so it will become more clear?
> > Currently ioctl works fine if driver is registered with "0000" permission
> > mode.
> >
> > Best regards,
> > Petro
> >
> > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xi...@gmail.com> пише:
> >>
> >> If we want to do the correct permission check, the ioctl handler needs to
> >> check R/W bit by itself based on how the ioctl is implemented.
> >> Or follow up how Linux encode the needed permission into each IOCTL:
> >> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> >> and let's VFS layer do the check for each driver.
> >>
> >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> >> petro.karashchenko@gmail.com> wrote:
> >>
> >> > Hello team,
> >> >
> >> > Recently I have noticed that there are many places in code where
> >> > register_driver() is called with non-zero mode with file operation
> >> > structures that have neither read nor write APIs implemented. For
> >> > example "ret = register_driver(path, &opamp_fops, 0444, dev);" while
> >> > opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
> >> > implemented. I made a PR to fix it
> >> > https://github.com/apache/incubator-nuttx/pull/5347 and change mode
> >> > from "0444" to "0000", but want to ask if anyone sees any drawback in
> >> > such an approach? Maybe I'm missing something?
> >> >
> >> > Best regards,
> >> > Petro
> >> >
> >
Re: register_driver with 0000 mode
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
Hi Petro,
The read permission is needed even when you just want to open a file:
$ vim noreadfile
$ chmod 0000 noreadfile
$ ls -l noreadfile
---------- 1 user user 5 jan 28 09:24 noreadfile
$ cat noreadfile
cat: noreadfile: Permission denied
You can even try to create a C program just to open it, and it will fail.
See the man page of open function:
The argument flags *must* include one of the following access
modes: O_RDONLY, O_WRONLY, or
O_RDWR. These request opening the file read-only, write-only,
or read/write, respectively.
This man page makes it clear you must include an access mode, but I
passed 0 to the access mode flag of open() and it was accepted, but
when the file has permission 0000 it returns -EPERM: "Failed to open
file: error -1"
BR,
Alan
On 1/28/22, Petro Karashchenko <pe...@gmail.com> wrote:
> Hello,
>
> Yes, but how does this relate to "0000" mode for "register_driver()"?
> Maybe you can describe some use case so it will become more clear?
> Currently ioctl works fine if driver is registered with "0000" permission
> mode.
>
> Best regards,
> Petro
>
> пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xi...@gmail.com> пише:
>>
>> If we want to do the correct permission check, the ioctl handler needs to
>> check R/W bit by itself based on how the ioctl is implemented.
>> Or follow up how Linux encode the needed permission into each IOCTL:
>> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
>> and let's VFS layer do the check for each driver.
>>
>> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
>> petro.karashchenko@gmail.com> wrote:
>>
>> > Hello team,
>> >
>> > Recently I have noticed that there are many places in code where
>> > register_driver() is called with non-zero mode with file operation
>> > structures that have neither read nor write APIs implemented. For
>> > example "ret = register_driver(path, &opamp_fops, 0444, dev);" while
>> > opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
>> > implemented. I made a PR to fix it
>> > https://github.com/apache/incubator-nuttx/pull/5347 and change mode
>> > from "0444" to "0000", but want to ask if anyone sees any drawback in
>> > such an approach? Maybe I'm missing something?
>> >
>> > Best regards,
>> > Petro
>> >
>
Re: register_driver with 0000 mode
Posted by Xiang Xiao <xi...@gmail.com>.
On Fri, Jan 28, 2022 at 6:05 PM Petro Karashchenko <
petro.karashchenko@gmail.com> wrote:
> Hello,
>
> Yes, but how does this relate to "0000" mode for "register_driver()"?
>
"0000" means no read/write/execute permission, I can't imagine any real
driver could benefit from it.
> Maybe you can describe some use case so it will become more clear?
> Currently ioctl works fine if driver is registered with "0000" permission
> mode.
>
Yes, because no place checks the permission in ioctl code path, but it is
the wrong implementation. Since different IOCTL does the different
thing(e.g. query/change state or get version number), the
correct implementation should check the permission case by case, for
example:
1. FIOC_REFORMAT needs check write permission
2. FIONREAD needs check read permission
Since NuttX doesn't encode the required permissions into ioctl code, the
check has to be done by an individual ioctl handler.
> Best regards,
> Petro
>
> пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xi...@gmail.com> пише:
> >
> > If we want to do the correct permission check, the ioctl handler needs to
> > check R/W bit by itself based on how the ioctl is implemented.
> > Or follow up how Linux encode the needed permission into each IOCTL:
> >
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> > and let's VFS layer do the check for each driver.
> >
> > On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> > petro.karashchenko@gmail.com> wrote:
> >
> > > Hello team,
> > >
> > > Recently I have noticed that there are many places in code where
> > > register_driver() is called with non-zero mode with file operation
> > > structures that have neither read nor write APIs implemented. For
> > > example "ret = register_driver(path, &opamp_fops, 0444, dev);" while
> > > opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
> > > implemented. I made a PR to fix it
> > > https://github.com/apache/incubator-nuttx/pull/5347 and change mode
> > > from "0444" to "0000", but want to ask if anyone sees any drawback in
> > > such an approach? Maybe I'm missing something?
> > >
> > > Best regards,
> > > Petro
> > >
>
Re: register_driver with 0000 mode
Posted by Petro Karashchenko <pe...@gmail.com>.
Hello,
Yes, but how does this relate to "0000" mode for "register_driver()"?
Maybe you can describe some use case so it will become more clear?
Currently ioctl works fine if driver is registered with "0000" permission mode.
Best regards,
Petro
пт, 28 січ. 2022 р. о 11:39 Xiang Xiao <xi...@gmail.com> пише:
>
> If we want to do the correct permission check, the ioctl handler needs to
> check R/W bit by itself based on how the ioctl is implemented.
> Or follow up how Linux encode the needed permission into each IOCTL:
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
> and let's VFS layer do the check for each driver.
>
> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
> petro.karashchenko@gmail.com> wrote:
>
> > Hello team,
> >
> > Recently I have noticed that there are many places in code where
> > register_driver() is called with non-zero mode with file operation
> > structures that have neither read nor write APIs implemented. For
> > example "ret = register_driver(path, &opamp_fops, 0444, dev);" while
> > opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
> > implemented. I made a PR to fix it
> > https://github.com/apache/incubator-nuttx/pull/5347 and change mode
> > from "0444" to "0000", but want to ask if anyone sees any drawback in
> > such an approach? Maybe I'm missing something?
> >
> > Best regards,
> > Petro
> >
Re: register_driver with 0000 mode
Posted by Xiang Xiao <xi...@gmail.com>.
If we want to do the correct permission check, the ioctl handler needs to
check R/W bit by itself based on how the ioctl is implemented.
Or follow up how Linux encode the needed permission into each IOCTL:
https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
and let's VFS layer do the check for each driver.
On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko <
petro.karashchenko@gmail.com> wrote:
> Hello team,
>
> Recently I have noticed that there are many places in code where
> register_driver() is called with non-zero mode with file operation
> structures that have neither read nor write APIs implemented. For
> example "ret = register_driver(path, &opamp_fops, 0444, dev);" while
> opamp_fops has only "opamp_open", "opamp_close" and "opamp_ioctl"
> implemented. I made a PR to fix it
> https://github.com/apache/incubator-nuttx/pull/5347 and change mode
> from "0444" to "0000", but want to ask if anyone sees any drawback in
> such an approach? Maybe I'm missing something?
>
> Best regards,
> Petro
>