Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Miklos Szeredi]

On Thu, Apr 19, 2018 at 9:54 PM, Vivek Goyal  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  wrote:
>> > On Wed, 18 Apr 2018 13:42:03 +0200
>> > Miklos Szeredi  wrote:
>> >
>> >> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  
>> >> wrote:
>> >> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
>> >> > 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-songliubrav...@fb.com
>> >
>> > 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

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Miklos Szeredi]

On Thu, Apr 19, 2018 at 9:54 PM, Vivek Goyal  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  wrote:
>> > On Wed, 18 Apr 2018 13:42:03 +0200
>> > Miklos Szeredi  wrote:
>> >
>> >> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  
>> >> wrote:
>> >> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
>> >> > 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-songliubrav...@fb.com
>> >
>> > 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

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Vivek Goyal]

On Wed, Apr 18, 2018 at 03:49:02PM +0200, Miklos Szeredi wrote:
> On Wed, Apr 18, 2018 at 3:38 PM, Steven Rostedt  wrote:
> > On Wed, 18 Apr 2018 13:42:03 +0200
> > Miklos Szeredi  wrote:
> >
> >> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  
> >> wrote:
> >> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
> >> > 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-songliubrav...@fb.com
> >
> > 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.

Am I missing the point completely.

Thanks
Vivek

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Vivek Goyal]

On Wed, Apr 18, 2018 at 03:49:02PM +0200, Miklos Szeredi wrote:
> On Wed, Apr 18, 2018 at 3:38 PM, Steven Rostedt  wrote:
> > On Wed, 18 Apr 2018 13:42:03 +0200
> > Miklos Szeredi  wrote:
> >
> >> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  
> >> wrote:
> >> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
> >> > 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-songliubrav...@fb.com
> >
> > 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.

Am I missing the point completely.

Thanks
Vivek

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Steven Rostedt]

On Wed, 18 Apr 2018 15:49:02 +0200
Miklos Szeredi  wrote:


> > I just received this patch:
> >
> >  http://lkml.kernel.org/r/20180418062907.3210386-1-songliubrav...@fb.com
> >
> > 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'll review, but apparently I wasn't CC-d on that patch.   Weird.

Especially since you are on the "Reported-by".

My scripts know to add to the Cc "Reported-by" tags. Not all scripts do
though :-/

-- Steve

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Steven Rostedt]

On Wed, 18 Apr 2018 15:49:02 +0200
Miklos Szeredi  wrote:


> > I just received this patch:
> >
> >  http://lkml.kernel.org/r/20180418062907.3210386-1-songliubrav...@fb.com
> >
> > 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'll review, but apparently I wasn't CC-d on that patch.   Weird.

Especially since you are on the "Reported-by".

My scripts know to add to the Cc "Reported-by" tags. Not all scripts do
though :-/

-- Steve

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Miklos Szeredi]

On Wed, Apr 18, 2018 at 3:38 PM, Steven Rostedt  wrote:
> On Wed, 18 Apr 2018 13:42:03 +0200
> Miklos Szeredi  wrote:
>
>> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  wrote:
>> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
>> > 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-songliubrav...@fb.com
>
> 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'll review, but apparently I wasn't CC-d on that patch.   Weird.

Thanks,
Miklos

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Miklos Szeredi]

On Wed, Apr 18, 2018 at 3:38 PM, Steven Rostedt  wrote:
> On Wed, 18 Apr 2018 13:42:03 +0200
> Miklos Szeredi  wrote:
>
>> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  wrote:
>> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
>> > 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-songliubrav...@fb.com
>
> 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'll review, but apparently I wasn't CC-d on that patch.   Weird.

Thanks,
Miklos

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Steven Rostedt]

On Wed, 18 Apr 2018 13:42:03 +0200
Miklos Szeredi  wrote:

> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  wrote:
> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
> > 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.

> >
> > 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-songliubrav...@fb.com

Which removes this code. Can you review it and I'll take it.

-- Steve

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Steven Rostedt]

On Wed, 18 Apr 2018 13:42:03 +0200
Miklos Szeredi  wrote:

> On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  wrote:
> > On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  
> > 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.

> >
> > 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-songliubrav...@fb.com

Which removes this code. Can you review it and I'll take it.

-- Steve

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Miklos Szeredi]

On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  wrote:
> On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  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?
>
> 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.

Thanks,
Miklos

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Miklos Szeredi]

On Wed, Apr 18, 2018 at 10:19 AM, Amir Goldstein  wrote:
> On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  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?
>
> 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.

Thanks,
Miklos

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Amir Goldstein]

On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  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?

Not sure yet if overlayfs-rorw patches would allow reverting this
change.

Thanks,
Amir.

Re: [RFC PATCH 31/35] Revert "vfs: add d_real_inode() helper"

[Amir Goldstein]

On Thu, Apr 12, 2018 at 6:08 PM, Miklos Szeredi  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?

Not sure yet if overlayfs-rorw patches would allow reverting this
change.

Thanks,
Amir.