On Thu, Apr 19, 2018 at 9:54 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > On Wed, Apr 18, 2018 at 03:49:02PM +0200, Miklos Szeredi wrote: >> On Wed, Apr 18, 2018 at 3:38 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: >> > On Wed, 18 Apr 2018 13:42:03 +0200 >> > Miklos Szeredi <miklos@xxxxxxxxxx> wrote: >> > >> >> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> >> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi <mszeredi@xxxxxxxxxx> wrote: >> >> >> This reverts commit a118084432d642eeccb961c7c8cc61525a941fcb. >> >> >> >> >> >> No user of d_real_inode() remains, so it can be removed. >> >> >> >> >> > >> >> > FYI, there is a new user in v4.17-rc1 added by commit >> >> > f0a2aa5a2a40 tracing/uprobe: Add support for overlayfs >> >> > >> >> > Seems like this patch got merged without any CC to overlayfs >> >> > mailing list nor maintainer? >> > >> > It appeared to be a small change with lots of reviewers. I didn't think >> > it was something to notify the overlayfs folks with. But perhaps I was >> > wrong. >> >> The patch is correct. The code surrounding it isn't, though. >> >> > >> >> > >> >> > Not sure yet if overlayfs-rorw patches would allow reverting this >> >> > change. >> >> >> >> Not trivial, because uprobe is looking at i_mapping to get a list of >> >> current memory maps. We could set i_mapping at overlay inode >> >> initialization time, but we definitely can't *change* i_mapping at >> >> copy up. Which is bound to result in some weird inconsistencies. So >> >> likely we'll need to keep d_real_inode() for the time being. >> > >> > I just received this patch: >> > >> > http://lkml.kernel.org/r/20180418062907.3210386-1-songliubraving@xxxxxx >> > >> > Which removes this code. Can you review it and I'll take it. >> >> It shouldn't remove d_real_inode(), because that part is correct and >> fixes a real bug in handling overlayfs files. > > I am wondering what does it practically mean for metdata only copy up > patches. Given this is uprobe code, I am assuming its modifying some > executable code dynamically. And for the the case of metadata only > copy up, it will return inode of lower. That probably means, as long > as all running instances of that exeutable are using that inode, things > will work fine. > > But if for some reason somebody opens that file for WRITE and triggers > copy up and new instances of same binary will not see the probe taking > affect? > > Which is probably very similar to what will happen if a lower executable > is copied up. Having said that, in normal cases there should not be a > need to copy up a binary in normal circumstances. The only thing we need to ensure when uprobes interact with copy-ups is that the kernel doesn't crash and doesn't leak memory. Other than that, it's a totally uninteresting corner case and we don't need to worry about its behavior. Thanks, Miklos
