Re: [PATCH v2] fsnotify, lsm: Decouple fsnotify from lsm

[Amir Goldstein]

On Sun, Oct 13, 2024 at 4:46 PM Song Liu <songliubraving@xxxxxxxx> wrote:
>
> Hi Amir,
>
> > On Oct 13, 2024, at 2:38 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Sun, Oct 13, 2024 at 2:23 AM Song Liu <song@xxxxxxxxxx> wrote:
> >>
> >> Currently, fsnotify_open_perm() is called from security_file_open(). This
> >> is not right for CONFIG_SECURITY=n and CONFIG_FSNOTIFY=y case, as
> >> security_file_open() in this combination will be a no-op and not call
> >> fsnotify_open_perm(). Fix this by calling fsnotify_open_perm() directly.
> >
> > Maybe I am missing something.
> > I like cleaner interfaces, but if it is a report of a problem then
> > I do not understand what the problem is.
> > IOW, what does "This is not right" mean?
>
> With existing code, CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on
> CONFIG_SECURITY, but CONFIG_FSNOTIFY does not depend on
> CONFIG_SECURITY. So CONFIG_SECURITY=n and CONFIG_FSNOTIFY=y is a
> valid combination. fsnotify_open_perm() is an fsnotify API, so I
> think it is not right to skip the API call for this config.
>
> >
> >>
> >> After this, CONFIG_FANOTIFY_ACCESS_PERMISSIONS does not require
> >> CONFIG_SECURITY any more. Remove the dependency in the config.
> >>
> >> Signed-off-by: Song Liu <song@xxxxxxxxxx>
> >> Acked-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> >>
> >> ---
> >>
> >> v1: https://lore.kernel.org/linux-fsdevel/20241011203722.3749850-1-song@xxxxxxxxxx/
> >>
> >> As far as I can tell, it is necessary to back port this to stable. Because
> >> CONFIG_FANOTIFY_ACCESS_PERMISSIONS is the only user of fsnotify_open_perm,
> >> and CONFIG_FANOTIFY_ACCESS_PERMISSIONS depends on CONFIG_SECURITY.
> >> Therefore, the following tags are not necessary. But I include here as
> >> these are discussed in v1.
> >
> > I did not understand why you claim that the tags are or not necessary.
> > The dependency is due to removal of the fsnotify.h include.
>
> I think the Fixes tag is also not necessary, not just the two
> Depends-on tags. This is because while fsnotify_open_perm() is a
> fsnotify API, only CONFIG_FANOTIFY_ACCESS_PERMISSIONS really uses
> (if I understand correctly).
>

That is correct.

> >
> > Anyway, I don't think it is critical to backport this fix.
> > The dependencies would probably fail to apply cleanly to older kernels,
> > so unless somebody cares, it would stay this way.
>
> I agree it is not critical to back port this fix. I put the
> Fixes tag below "---" for this reason.
>
> Does this answer your question?
>

Yes, I agree with not including any of the tags and not targeting stable.

Jan, Christian,

do you agree with the wording of the commit message, or think
that it needs to be clarified?

Would you prefer this to go via the fsnotify tree or vfs tree?

Thanks,
Amir.