Re: [PATCH v5 2/7] landlock: Add IOCTL access right

["Günther Noack" <gnoack@google.com>]

Hello!

On Fri, Nov 17, 2023 at 09:45:47PM +0100, Mickaël Salaün wrote:
> On Fri, Nov 17, 2023 at 04:49:15PM +0100, Günther Noack wrote:
> > +static inline access_mask_t expand_ioctl(const access_mask_t handled,
> 
> Please remove all explicit inlines in the .c files, the compiler should
> be able to inline them if necessary, or it my not inline them at all
> anyway.  It would be nice to check the -O2 code to see what GCC or clang
> do though, and I guess they will inline this kind of pattern.

Done.


> > +					 const access_mask_t access,
> > +					 const access_mask_t src,
> > +					 const access_mask_t dst)
> > +{
> > +	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> > +		return 0;
> > +
> > +	access_mask_t copy_from = (handled & src) ? src :
> 
> Please declare variables at the beginning of blocks.

Done.


> > +static inline access_mask_t
> > +landlock_expand_access_fs(const access_mask_t handled,
> > +			  const access_mask_t access)
> > +{
> > +	return access |
> > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP4) |
> > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE,
> > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP3) |
> > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR,
> > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1);
> > +}
> 
> I'd prefer to keep the semantic definition of these groups (i.e.
> required_ioctl_access) close the definition of access right expantions,
> and also close to the ioctl_groups veriable. Actually, ioctl_groups
> might make more sense close to the group definition, and then probably
> another define... What do you think?

Done, good idea. Thanks for the suggestion!

I think this makes sense and the code looks better when the IOCTL-related
functionality is grouped together at the top of fs.c, including ioctl_groups,
the LANDLOCK_ACCESS_FS_IOCTL_GROUP1,2,3,4 #defines, the expansion helpers and
the required_ioctl_access helper.


> > +	/*
> > +	 * It is the access rights at the time of opening the file which
> > +	 * determine whether ioctl can be used on the opened file later.
> 
> s/ioctl/IOCTL/g

Done.


> > +/**
> > + * landlock_expand_handled_access_fs() - add synthetic IOCTL access rights to an
> > + * access mask of handled accesses.
> > + *
> > + * @handled: The handled accesses of a ruleset that is being created
> > + *
> > + * Returns: @handled, with the bits for the synthetic IOCTL access rights set,
> > + * if %LANDLOCK_ACCESS_FS_IOCTL is handled
> > + */
> 
> This doc should be in fs.c

Done, thanks.

(I did not realize that the convention worked this way in the kernel.  This goes
on my bucket list of things to double check.)

Thanks for the review!

—Günther