Re: [PATCH 16/23] initramfs: simplify clean_rootfs

It was <2020-07-23 czw 11:22>, when Christoph Hellwig wrote:

On Sat, Jul 18, 2020 at 12:00:35PM +0200, Christoph Hellwig wrote:

quoted

On Fri, Jul 17, 2020 at 10:55:48PM +0200, Marek Szyprowski wrote:

quoted

On 14.07.2020 21:04, Christoph Hellwig wrote:

quoted

Just use d_genocide instead of iterating through the root directory
with cumbersome userspace-like APIs.  This also ensures we actually
remove files that are not direct children of the root entry, which
the old code failed to do.

Fixes: df52092f3c97 ("fastboot: remove duplicate unpack_to_rootfs()")
Signed-off-by: Christoph Hellwig <hch@lst.de>

This patch breaks initrd support ;-(

I use initrd to deploy kernel modules on my test machines. It was
automatically mounted on /initrd. /lib/modules is just a symlink to
/initrd. I know that initrd support is marked as deprecated, but it
would be really nice to give people some time to update their
machines before breaking the stuff.

Looks like your setup did rely on the /dev/ notes from the built-in
initramfs to be preserved.

Our initrd image contains only the modules directory and 5.8.0-rc5-next-20200717
in it.

quoted

Can you comment out the call to d_genocide?  It seems like for your
the fact that clean_rootfs didn't actually clean up was a feature and
not a bug.

I guess the old, pre-2008 code also wouldn't have worked for you in
that case.

Did you get a chance to try this?

Indeed, commenting out d_genocide() helps.

Kind regards,
ŁS

PS. Marek is currently out of office.
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

On Thu, Jul 23, 2020 at 04:25:34PM +0200, Lukasz Stelmach wrote:

quoted

quoted

Can you comment out the call to d_genocide?  It seems like for your
the fact that clean_rootfs didn't actually clean up was a feature and
not a bug.

I guess the old, pre-2008 code also wouldn't have worked for you in
that case.

Did you get a chance to try this?

Indeed, commenting out d_genocide() helps.

So given that people have relied on at least the basic device nodes
like /dev/console to not go away since 2008, I wonder if we should just
remove clean_rootfs entirely

Linus, Al?

On Thu, Jul 23, 2020 at 04:27:34PM +0200, Christoph Hellwig wrote:

On Thu, Jul 23, 2020 at 04:25:34PM +0200, Lukasz Stelmach wrote:

quoted

quoted

quoted

Can you comment out the call to d_genocide?  It seems like for your
the fact that clean_rootfs didn't actually clean up was a feature and
not a bug.

I guess the old, pre-2008 code also wouldn't have worked for you in
that case.

Did you get a chance to try this?

Indeed, commenting out d_genocide() helps.

So given that people have relied on at least the basic device nodes
like /dev/console to not go away since 2008, I wonder if we should just
remove clean_rootfs entirely

Linus, Al?

First of all, d_genocide() is simply wrong here from VFS point of view.  _IF_
you want recursive removal, you need simple_recursive_remove(path.dentry, NULL).
And it's a userland-visible change of behaviour.

As for removal of clean_rootfs()...  FWIW, the odds of an image that would
eventually fail accidentally getting past the signature mismatch check are
fairly low.  I've no idea what scenario the author of that thing used to have;
that would be Shaohua Li [off-list ref].  Cc'd...

On Mon, Jul 27, 2020 at 03:41:49AM +0100, Al Viro wrote:

On Thu, Jul 23, 2020 at 04:27:34PM +0200, Christoph Hellwig wrote:

quoted

On Thu, Jul 23, 2020 at 04:25:34PM +0200, Lukasz Stelmach wrote:

quoted

quoted

quoted

Can you comment out the call to d_genocide?  It seems like for your
the fact that clean_rootfs didn't actually clean up was a feature and
not a bug.

I guess the old, pre-2008 code also wouldn't have worked for you in
that case.

Did you get a chance to try this?

Indeed, commenting out d_genocide() helps.

So given that people have relied on at least the basic device nodes
like /dev/console to not go away since 2008, I wonder if we should just
remove clean_rootfs entirely

Linus, Al?

First of all, d_genocide() is simply wrong here from VFS point of view.  _IF_
you want recursive removal, you need simple_recursive_remove(path.dentry, NULL).
And it's a userland-visible change of behaviour.

As for removal of clean_rootfs()...  FWIW, the odds of an image that would
eventually fail accidentally getting past the signature mismatch check are
fairly low.  I've no idea what scenario the author of that thing used to have;
that would be Shaohua Li [off-list ref].  Cc'd...

Shaohua is now at Facebook.