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

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

[Matthew Bobrowski]

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

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.

Should we also be CC'ing <stable@vger.kernel.org> so this gets backported?

 fs/notify/fanotify/fanotify_user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index be5b6d2c01e7..64864fb40b40 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -471,7 +471,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 					info_type, fanotify_info_name(info),
 					info->name_len, buf, count);
 		if (ret < 0)
-			return ret;
+			goto out_close_fd;
 
 		buf += ret;
 		count -= ret;
@@ -519,7 +519,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 					fanotify_event_object_fh(event),
 					info_type, dot, dot_len, buf, count);
 		if (ret < 0)
-			return ret;
+			goto out_close_fd;
 
 		buf += ret;
 		count -= ret;
-- 
2.32.0.272.g935e593368-goog

/M

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

[Amir Goldstein]

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.

If you want to do a service to the 5.4.y downstream community,
you can send a backport patch directly to stable list *after* this patch
is applied to master.

>
> 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).

But if you do specify stable kernel version hint, try not to get it wrong
like I did :-/
https://lore.kernel.org/linux-fsdevel/20210608122829.GI5562@quack2.suse.cz/

CC-ing Greg in case my understanding of the stable kernel patch
candidate analysis process is wrong.

Thanks,
Amir.

>  fs/notify/fanotify/fanotify_user.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index be5b6d2c01e7..64864fb40b40 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -471,7 +471,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>                                         info_type, fanotify_info_name(info),
>                                         info->name_len, buf, count);
>                 if (ret < 0)
> -                       return ret;
> +                       goto out_close_fd;
>
>                 buf += ret;
>                 count -= ret;
> @@ -519,7 +519,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>                                         fanotify_event_object_fh(event),
>                                         info_type, dot, dot_len, buf, count);
>                 if (ret < 0)
> -                       return ret;
> +                       goto out_close_fd;
>
>                 buf += ret;
>                 count -= ret;
> --
> 2.32.0.272.g935e593368-goog
>
> /M

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

[Greg KH]

On Fri, Jun 11, 2021 at 10:04:06AM +0300, 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.
> 
> If you want to do a service to the 5.4.y downstream community,
> you can send a backport patch directly to stable list *after* this patch
> is applied to master.
> 
> >
> > 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).
> 
> But if you do specify stable kernel version hint, try not to get it wrong
> like I did :-/
> https://lore.kernel.org/linux-fsdevel/20210608122829.GI5562@quack2.suse.cz/
> 
> CC-ing Greg in case my understanding of the stable kernel patch
> candidate analysis process is wrong.

Nope, that's right, and splitting this up would have been great, but we
can deal with it.

thanks,

greg k-h

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

[Jan Kara]

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

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

[Matthew Bobrowski]

On Mon, Jun 14, 2021 at 12:28:42PM +0200, Jan Kara wrote:
> 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:
> > 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.

Ah, I see. Thanks for providing your perspectives and sharing your
knowledge.

> 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...

Thanks Jan!

/M

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

[Greg KH]

On Tue, Jun 15, 2021 at 07:24:32PM +1000, Matthew Bobrowski wrote:
> On Mon, Jun 14, 2021 at 12:28:42PM +0200, Jan Kara wrote:
> > 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:
> > > 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.

No, it is NOT.

We have to pick up the "Fixes:" stuff because of maintainers and
developers that forget to use Cc: stable like has been documented.

But we don't always do it as quickly as a cc: stable line will offer.
And sometimes we don't get to those at all.

So if you know it needs to go to a stable kernel, ALWAYS put a cc:
stable as the documentation says to do so.  This isn't a new
requirement, it's been this way for 17 years now!

thanks,

greg k-h

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

[Jan Kara]

On Tue 15-06-21 11:41:53, Greg KH wrote:
> On Tue, Jun 15, 2021 at 07:24:32PM +1000, Matthew Bobrowski wrote:
> > On Mon, Jun 14, 2021 at 12:28:42PM +0200, Jan Kara wrote:
> > > 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:
> > > > 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.
> 
> No, it is NOT.
> 
> We have to pick up the "Fixes:" stuff because of maintainers and
> developers that forget to use Cc: stable like has been documented.
> 
> But we don't always do it as quickly as a cc: stable line will offer.
> And sometimes we don't get to those at all.
> 
> So if you know it needs to go to a stable kernel, ALWAYS put a cc:
> stable as the documentation says to do so.  This isn't a new
> requirement, it's been this way for 17 years now!

OK, as I said I do add cc: stable when I think the patch should go to
stable. But practically patches with the Fixes tag get to stable so
reliably that I was suspecting you actually have a bot processing Linus'
tree and forwarding all patches with Fixes tag to stable as well :) If
that's not the case, I'm sorry for misguiding Matthew.

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

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

[Greg KH]

On Tue, Jun 15, 2021 at 12:23:47PM +0200, Jan Kara wrote:
> On Tue 15-06-21 11:41:53, Greg KH wrote:
> > On Tue, Jun 15, 2021 at 07:24:32PM +1000, Matthew Bobrowski wrote:
> > > On Mon, Jun 14, 2021 at 12:28:42PM +0200, Jan Kara wrote:
> > > > 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:
> > > > > 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.
> > 
> > No, it is NOT.
> > 
> > We have to pick up the "Fixes:" stuff because of maintainers and
> > developers that forget to use Cc: stable like has been documented.
> > 
> > But we don't always do it as quickly as a cc: stable line will offer.
> > And sometimes we don't get to those at all.
> > 
> > So if you know it needs to go to a stable kernel, ALWAYS put a cc:
> > stable as the documentation says to do so.  This isn't a new
> > requirement, it's been this way for 17 years now!
> 
> OK, as I said I do add cc: stable when I think the patch should go to
> stable. But practically patches with the Fixes tag get to stable so
> reliably that I was suspecting you actually have a bot processing Linus'
> tree and forwarding all patches with Fixes tag to stable as well :) If
> that's not the case, I'm sorry for misguiding Matthew.

We have scripts for this, but they usually run after all of the cc:
stable patches have been processed, so there is a delay, if they run at
all.

Again, the rules have not changed for 17 years, please add a cc: stable
on the signed-off-by area if you want to be sure to get your patches
merged to a stable tree.  Any patch not doing that is not guaranteed to
have it merged there and are at the whim of Sasha and mine scripts...

thanks,

greg k-h