Re: [PATCH] fanotify.7: Document extended response to permission events

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Mar 30, 2025 at 7:52 PM Alejandro Colomar <alx@xxxxxxxxxx> wrote:
>
> Hi Amir,
>
> On Sun, Mar 30, 2025 at 05:33:26PM +0200, Amir Goldstein wrote:
> > Document FAN_DENY_ERRNO(), that was added in v6.13 and the
> > FAN_RESPONSE_INFO_AUDIT_RULE extended response info record
> > that was added in v6.3.
> >
> > Cc: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> >
> > Alejandro,
> >
> > I was working on man page updates to fanotify features that landed
> > in v6.14 and found a few bits from v6.3 that were out of date, so
> > I added them along with this change.
> >
> > If you want me to split them out I can, but I did not see much point.
>
> I prefer them in two patches.  You can send them in the same patch set,
> though.

ok

I pushed the two patches to
https://github.com/amir73il/man-pages/commits/fan_deny_errno

Let me know if you want me to re-post them

>
> > This change to the documentation of fanotify permission event response
> > is independent of the previous patches I posted to document the new
> > FAN_PRE_ACCESS event (also v6.14) and the fanotify_init(2) flag
> > FAN_REPORT_FD_ERROR (v6.13).
> >
> > There is another fanotify feature in v6.14 (mount events).
> > I will try to catch up on documenting that one as well.
> >
> > Thanks,
> > Amir.
> >
> >  man/man7/fanotify.7 | 60 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/man/man7/fanotify.7 b/man/man7/fanotify.7
> > index 6f3a9496e..c7b53901a 100644
> > --- a/man/man7/fanotify.7
> > +++ b/man/man7/fanotify.7
> > @@ -820,7 +820,7 @@ This is the file descriptor from the structure
> >  .TP
> >  .I response
> >  This field indicates whether or not the permission is to be granted.
> > -Its value must be either
> > +Its value must contain either the flag
>
> This seems unrelated.  Please keep it out of the patches.  If you want
> to do it, please have a third trivial patch with "wfix" in the subject.

what does wfix stand for?

this is not a typo fix, this is a semantic fix.

It is not true that the value of response is either FAN_ALLOW or FAN_DENY
those are flags in a bitset and the correct statement is that exactly
one of them needs to be set.

>
> >  .B FAN_ALLOW
> >  to allow the file operation or
> >  .B FAN_DENY
> > @@ -829,6 +829,24 @@ to deny the file operation.
> >  If access is denied, the requesting application call will receive an
> >  .B EPERM
> >  error.
> > +Since Linux 6.14,
> > +.\" commit b4b2ff4f61ded819bfa22e50fdec7693f51cbbee
> > +if a notification group is initialized with class
> > +.BR FAN_CLASS_PRE_CONTENT ,
> > +the following error values could be returned to the application
> > +by setting the
> > +.I response
> > +value using the
> > +.BR FAN_DENY_ERRNO(err)
>
> This formatting is incorrect.  BR means alternating Bold and Roman, but
> this only has one token.
>

ok I added a space

> > +macro:
> > +.BR EPERM ,
> > +.BR EIO ,
> > +.BR EBUSY ,
> > +.BR ETXTBSY ,
> > +.BR EAGAIN ,
> > +.BR ENOSPC ,
> > +.BR EDQUOT .
>
> Should we have a manual page for FAN_DENY_ERRNO()?  (I think we should.)
> I don't understand how it's supposed to work from this paragraph.
>


#define FAN_DENY_ERRNO(err) (FAN_DENY | (((err) & 0xff) << 24))

combined FAN_DENY with a specific error, but I see no
reason to expose the internals of this macro

This does not deserve a man page of its own IMO.

If you have a suggested for better formatting, please suggest it


> > +.P
> >  Additionally, if the notification group has been created with the
> >  .B FAN_ENABLE_AUDIT
> >  flag, then the
> > @@ -838,6 +856,46 @@ flag can be set in the
> >  field.
> >  In that case, the audit subsystem will log information about the access
> >  decision to the audit logs.
>
> Do we want to start a new paragraph maybe?
>

ok

> > +Since Linux 6.3,
> > +.\" commit 70529a199574c15a40f46b14256633b02ba10ca2
> > +the
> > +.B FAN_INFO
> > +flag can be set in the
> > +.I response
> > +to indicate that extra variable length response record follows struct
>
> s/variable length/variable-length/
>
> And we usually say 'XXX structure' instead of 'struct XXX'.
>

ok

> > +.IR fanotify_response .
>
> The above sentence is too long.  I'd split it into two:
>
> Since Linux 6.3, the FAN_INFO flag can be set in the response field.  It
> indicates that an extra variable-length response record follows the
> fanotify_response structure.
>

ok

> > +Extra response records start with a common header:
> > +.P
> > +.in +4n
> > +.EX
> > +struct fanotify_response_info_header {
> > +    __u8 type;
> > +    __u8 pad;
> > +    __u16 len;
> > +};
> > +.EE
> > +.in
> > +.P
> > +The value of
> > +.I type
>
> I'd say '.type' instead of 'type'.  I know there's no consistency about
> it, but I'm going to globally fix that eventually.  Let's do it good for
> new documentation.  The '.' allows one to easily know that we're talking
> about a struct or union member.
>

Sounds like a good change to me.

pushed requested fixes to github.

Thanks,
Amir.





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux