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). > > 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? Thanks, Song > >> >> Fixes: c4ec54b40d33 ("fsnotify: new fsnotify hooks and events types for access decisions") > > Because I am not sure what the problem is, I am not sure that a Fixes: > tag is called for. > >> Depends-on: 36e28c42187c ("fsnotify: split fsnotify_perm() into two hooks") >> Depends-on: d9e5d31084b0 ("fsnotify: optionally pass access range in file permission hooks") > > These need to be in the commit message in case AUTOSEL or a developer > would decide to backport your change. > > Thanks, > Amir.
