| Message ID | 1ef8ae9100101eb1a91763c516c2e9a3a3b112bd.1623376346.git.repnop@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: fix copy_event_to_user() fid error clean up | expand |
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
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
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
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
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
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
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
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;
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(-)