Re: [PATCH] fanotify: fix copy_event_to_user() fid error clean up

[Jan Kara <jack@suse.cz>]

On Fri 11-06-21 10:04:06, Amir Goldstein wrote:
> On Fri, Jun 11, 2021 at 6:32 AM Matthew Bobrowski <repnop@google.com> wrote:
> >
> > Ensure that clean up is performed on the allocated file descriptor and
> > struct file object in the event that an error is encountered while copying
> > fid info objects. Currently, we return directly to the caller when an error
> > is experienced in the fid info copying helper, which isn't ideal given that
> > the listener process could be left with a dangling file descriptor in their
> > fdtable.
> >
> > Fixes: 44d705b0370b1 ("fanotify: report name info for FAN_DIR_MODIFY event")
> > Fixes: 5e469c830fdb5 ("fanotify: copy event fid info to user")
> > Link: https://lore.kernel.org/linux-fsdevel/YMKv1U7tNPK955ho@google.com/T/#m15361cd6399dad4396aad650de25dbf6b312288e
> >
> 
> This newline should not be here.
> 
> > Signed-off-by: Matthew Bobrowski <repnop@google.com>
> > ---
> >
> > Hey Amir/Jan,
> >
> > I wasn't 100% sure what specific commit hash I should be referencing in the
> > fix tags, so please let me know if that needs to be changed.
> 
> Trick question.
> There are two LTS kernels where those fixes are relevant 5.4.y and 5.10.y
> (Patch would be picked up for latest stable anyway)
> The first Fixes: suggests that the patch should be applied to 5.10+
> and the second Fixes: suggests that the patch should be applied to 5.4+
> 
> In theory, you could have split this to two patches, one auto applied to 5.4+
> and the other auto applied to +5.10.
> 
> In practice, this patch would not auto apply to 5.4.y cleanly even if you
> split it and also, it's arguably not that critical to worth the effort,
> so I would keep the first Fixes: tag and drop the second to avoid the
> noise of the stable bots trying to apply the patch.

Actually I'd rather keep both Fixes tags. I agree this patch likely won't
apply for older kernels but it still leaves the information which code is
being fixed which is still valid and useful. E.g. we have an
inftrastructure within SUSE that informs us about fixes that could be
applicable to our released kernels (based on Fixes tags) and we then
evaluate whether those fixes make sense for us and backport them.

> > Should we also be CC'ing <stable@vger.kernel.org> so this gets backported?
> >
> 
> Yes and no.
> Actually CC-ing the stable list is not needed, so don't do it.
> Cc: tag in the commit message is somewhat redundant to Fixes: tag
> these days, but it doesn't hurt to be explicit about intentions.
> Specifying:
>     Cc: <stable@vger.kernel.org> # v5.10+
> 
> Could help as a hint in case the Fixes: tags is for an old commit, but
> you know that the patch would not apply before 5.10 and you think it
> is not worth the trouble (as in this case).

I agree that CC to stable is more or less made redundant by the Fixes tag
these days. I still do use the CC tag for fixes where I think it is really
important they get pushed to stable or if there's not any particular
problematic commit that can be added to Fixes tag. But it's more or less
personal preference these days.

Anyway I've added the patch to my tree and will probably send it to Linus
later this week since the fix is trivial and obvious...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR