On Thu, Jun 10, 2021 at 10:11:51AM +0300, Amir Goldstein wrote: > > > > + ret = copy_info_records_to_user(event, info, info_mode, pidfd, > > > > + buf, count); > > > > if (ret < 0) > > > > - return ret; > > > > + goto out_close_fd; > > > > > > This looks like a bug in upstream. > > > > Yes, I'm glad this was picked up and I was actually wondering why it was > > acceptable to directly return without jumping to the out_close_fd label in > > the case of an error. I felt like it may have been a burden to raise the > > question in the first place because I thought that this got picked up in > > the review already and there was a good reason for having it, despite not > > really making much sense. > > > > > It should have been goto out_close_fd to begin with. > > > We did already copy metadata.fd to user, but the read() call > > > returns an error. > > > You should probably fix it before the refactoring patch, so it > > > can be applied to stable kernels. > > > > Sure, I will send through a patch fixing this before submitting the next > > version of this series though. How do I tag the patch so that it's picked > > up an back ported accordingly? > > > > The best option, in case this is a regression (it probably is) > is the Fixes: tag which is both a clear indication for stale > candidate patch tells the bots exactly which stable kernel the > patch should be applied to. > > Otherwise, you can Cc: stable (see examples in git) > and generally any commit title with the right keywords > 'fix' 'regression' 'bug' should be caught but the stable AI bots. Ah, OK, noted. > > > > } > > > > > > > > return metadata.event_len; > > > > @@ -558,6 +632,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > > > > put_unused_fd(fd); > > > > fput(f); > > > > } > > > > + > > > > + if (pidfd < 0) > > > > > > That condition is reversed. > > > We do not seem to have any test coverage for this error handling > > > Not so surprising that upstream had a bug... > > > > Sorry Amir, I don't quite understand what you mean by "That condition is > > reversed". Presumably you're referring to the fd != FAN_NOFD check and not > > pidfd < 0 here. > > > > IDGI, why is the init/cleanup code not as simple as > > int pidfd = FAN_NOPIDFD; > ... > out_close_fd: > ... > if (pidfd >= 0) > put_unused_fd(fd); You're missing nothing, it's me that's missing a few brain cells. Sorry, the context switching on my end is real and I had overlooked what you meant. But yes, this will most definitely work. /M
