On Fri, Oct 14, 2016 at 9:48 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Fri, Oct 14, 2016 at 7:03 AM, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: >> On Thu, Oct 13, 2016 at 7:37 AM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: >>> >>> Please pull from: >> >> No. >> >> Or rather, I pulled and then immediately unpulled. When I look at the >> diff, I saw an obvious bug in the very first chunk. I'm not going to >> pull something that is this obviously buggy and untested. >> >> Your change to fs/ioctl.c to add a -EXDEV error case very clearly >> leaks a reference to 'src_file'. > > On the charge of writing obviously buggy code I plead guilty :-/ > On the charge of not testing my code I plead not guilty. > I exercised the FICLONERANGE intensively with the xfstests clone test group > and never experienced any problem and any warning running with all > relevant kernel debugging enabled. > > So how come this leak went unnoticed? > Because fdget (__fget_light) does not take a reference if the fd is not shared. > > So what can we do to catch silly mistakes like this one earlier and > without relying > on Linus's spidy senses? > Writing xfstests to test all fd interfaces with cloned fds? Is this a > scalable solution? Well, I added an idle loop thread to xfs_io and sure enough it catches the leak in test generic/157 (Try cross-device reflink). I will post patches to fstests. Sorry... > Or maybe it's best to add a trivial CONFIG_DEBUG_FDGET that will always > skip the no refcount optimization? > > To me the second option seems preferred from engineering pov > I can post this simple patch if you guys agree to this solution. > >> >> It's too late in the merge window for this to be fixed up any more. >> This has been a painful enough merge window for me that I'm not going >> to play around with obviously buggy pull requests. >> > > It has been a rocky merge window and I apologize for adding this last straw > and pooping the party for the entire overlay pull request. > In the hope that this may help to sweeten your verdict - > I just got my hands on a brand new testing machine, which is dedicated > to stress testing file systems on the latest rc. > > Amir. > >> Linus >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
