So Alan do you suggest to remove inode_checkflags? On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis <acas...@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 <petro.karashche...@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 > > <petro.karashche...@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 <acas...@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.karashche...@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 <xiaoxiang781...@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.karashche...@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 > >> > >> > > >> > > > >> > > >
