[PATCH] tmpfs: generate random sb->s_uuid

[PATCH] tmpfs: generate random sb->s_uuid

[Amir Goldstein]

This is used by overlayfs to encode intrasystem unique file handles.

Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 mm/shmem.c | 1 +
 1 file changed, 1 insertion(+)

Miklos,

Attached the simple patch you suggested.
With this patch the unionmount-testsuite run:
./run --ov=0 --samefs

Passes with (default) tmpfs as samefs.

Hugh,

Can you please Ack.

FYI, both xfs and ubifs have patches queued to export thier on-disk
uuid to sb->s_uuid.

Thanks,
Amir.

diff --git a/mm/shmem.c b/mm/shmem.c
index e67d6ba..b73f832 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3761,6 +3761,7 @@ int shmem_fill_super(struct super_block *sb, void *data, int silent)
 #ifdef CONFIG_TMPFS_POSIX_ACL
 	sb->s_flags |= MS_POSIXACL;
 #endif
+	generate_random_uuid(sb->s_uuid);
 
 	inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0, VM_NORESERVE);
 	if (!inode)
-- 
2.7.4

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Christoph Hellwig]

On Fri, May 05, 2017 at 12:44:28PM +0300, Amir Goldstein wrote:
> This is used by overlayfs to encode intrasystem unique file handles.

I think generate_random_uuid is the wrong interface to spread.
The right one would be uuid_be_gen, although I'll still need
to audit why one uses get_random_bytes and the other prandom_bytes.

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Amir Goldstein]

On Fri, May 5, 2017 at 1:01 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 05, 2017 at 12:44:28PM +0300, Amir Goldstein wrote:
>> This is used by overlayfs to encode intrasystem unique file handles.
>
> I think generate_random_uuid is the wrong interface to spread.
> The right one would be uuid_be_gen, although I'll still need
> to audit why one uses get_random_bytes and the other prandom_bytes.
>

Christoph,

You're keep getting in my way with this uuid business ;-)

Seriously, one has to do things in certain order.
There is no reason to wait for the common uuid code to settle down before
fixing something as trivial as this and it makes very little sense IMO to use
uuid_be_gen() for sb->s_uuid as it is now.

I am working actively with you and Andy to sort out the uuid.h mess and
it even seems we are getting closer to consensus.

There is no real 'cost' in 'spreading' generate_random_uuid() to tmpfs
and I promise to post a patch to replace generate_random_uuid()
in tmpfs to the new API once it is ready.

Cheers,
Amir.

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Christoph Hellwig]

On Fri, May 05, 2017 at 01:20:11PM +0300, Amir Goldstein wrote:
> Seriously, one has to do things in certain order.

Yes… and I want to ensure that order.

> There is no reason to wait for the common uuid code to settle down before
> fixing something as trivial as this and it makes very little sense IMO to use
> uuid_be_gen() for sb->s_uuid as it is now.

There is.  generate_random_uuid needs to die, and adding more callers
isn't the way to go.  Same for ubifs btw.

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Richard Weinberger]

Am 09.05.2017 um 11:02 schrieb Christoph Hellwig:
> On Fri, May 05, 2017 at 01:20:11PM +0300, Amir Goldstein wrote:
>> Seriously, one has to do things in certain order.
> 
> Yes… and I want to ensure that order.
> 
>> There is no reason to wait for the common uuid code to settle down before
>> fixing something as trivial as this and it makes very little sense IMO to use
>> uuid_be_gen() for sb->s_uuid as it is now.
> 
> There is.  generate_random_uuid needs to die, and adding more callers
> isn't the way to go.  Same for ubifs btw.

I agree. Right now UBIFS uses generate_random_uuid() in both kernel and mkfs.ubifs.
Before we expose it to ->s_uuid, UBIFS should use the correct UUID generation functions.
Oleksij, please follow this discussion before you re-send your UBIFS patch.

Thanks,
//richard

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Amir Goldstein]

On Tue, May 9, 2017 at 12:18 PM, Richard Weinberger <richard@nod.at> wrote:
> Am 09.05.2017 um 11:02 schrieb Christoph Hellwig:
>> On Fri, May 05, 2017 at 01:20:11PM +0300, Amir Goldstein wrote:

>>
>>> There is no reason to wait for the common uuid code to settle down before
>>> fixing something as trivial as this and it makes very little sense IMO to use
>>> uuid_be_gen() for sb->s_uuid as it is now.
>>
>> There is.  generate_random_uuid needs to die, and adding more callers
>> isn't the way to go.

That's Christoph's opinion and its fine. I stated my opinion above.
It's up to tmpfs/vfs maintainers to make the call about my patch.

>>Same for ubifs btw.

Same in what regard?
ubifs has generate_random_uuid() since 2008.
When the cleanup patch to generate_random_uuid() will be ready,
it will fixup ubifs as well.

>
> I agree. Right now UBIFS uses generate_random_uuid() in both kernel and mkfs.ubifs.
> Before we expose it to ->s_uuid, UBIFS should use the correct UUID generation functions.

Richard, this makes no sense at all.
You have deployed filesystems whose UUID has already been set using
generate_random_uuid(). You are not going to reformat those filesystems
when generate_random_uuid() dies in the kernel, so how is this even related
to exporting the filesystem's UUID to s_uuid?

The changes that Christoph promotes to the kernel uuid common functions
are important because we have too many home brewed implementations of
uuid handling in the kernel. But making the code cleaner and more reusable
isn't going to change the on-disk uuid representation and for filesystems, it
probably won't even change the cpu uuid representation (i.e. in super block
structs).

Seriously, it's not my battle to fight.
I don't stand to loose anything if Oleksij's patch gets held hostage
for the uuid
cleanups, but I don't get the UBIFS maintainer POV on the matter at hand.

Amir.

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Richard Weinberger]

Amir,

Am 09.05.2017 um 11:54 schrieb Amir Goldstein:
>>> There is.  generate_random_uuid needs to die, and adding more callers
>>> isn't the way to go.
> 
> That's Christoph's opinion and its fine. I stated my opinion above.
> It's up to tmpfs/vfs maintainers to make the call about my patch.
> 
>>> Same for ubifs btw.
> 
> Same in what regard?
> ubifs has generate_random_uuid() since 2008.
> When the cleanup patch to generate_random_uuid() will be ready,
> it will fixup ubifs as well.
> 
>>
>> I agree. Right now UBIFS uses generate_random_uuid() in both kernel and mkfs.ubifs.
>> Before we expose it to ->s_uuid, UBIFS should use the correct UUID generation functions.
> 
> Richard, this makes no sense at all.
> You have deployed filesystems whose UUID has already been set using
> generate_random_uuid(). You are not going to reformat those filesystems
> when generate_random_uuid() dies in the kernel, so how is this even related
> to exporting the filesystem's UUID to s_uuid?

Well, maybe we need a new feature flag in UBIFS that the UUID is now usable.
Sure, in the best case we just expose the existing UUID to ->s_uuid and everything
is fine.

> The changes that Christoph promotes to the kernel uuid common functions
> are important because we have too many home brewed implementations of
> uuid handling in the kernel. But making the code cleaner and more reusable
> isn't going to change the on-disk uuid representation and for filesystems, it
> probably won't even change the cpu uuid representation (i.e. in super block
> structs).
> 
> Seriously, it's not my battle to fight.
> I don't stand to loose anything if Oleksij's patch gets held hostage
> for the uuid
> cleanups, but I don't get the UBIFS maintainer POV on the matter at hand.

My POV is easy, I'm nervous about all this changes and the rush behind them.
And when Christoph raises concerns, I'm especially careful.
That's why I'm taking the UBIFS ->s_uuid patch for 4.13 after I had enough
time to verify and think. And finally that's also why I asked Oleksij
to follow the discussion. If he can explain in detail why UBIFS does the right
thing I have a much better feeling. If I have to figure myself it takes time
which I don't have right now.

Thanks,
//richard

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Oleksij Rempel]

Hi Richard,

On 09.05.2017 12:09, Richard Weinberger wrote:
> Amir,
>
> Am 09.05.2017 um 11:54 schrieb Amir Goldstein:
>>>> There is.  generate_random_uuid needs to die, and adding more callers
>>>> isn't the way to go.
>>
>> That's Christoph's opinion and its fine. I stated my opinion above.
>> It's up to tmpfs/vfs maintainers to make the call about my patch.
>>
>>>> Same for ubifs btw.
>>
>> Same in what regard?
>> ubifs has generate_random_uuid() since 2008.
>> When the cleanup patch to generate_random_uuid() will be ready,
>> it will fixup ubifs as well.
>>
>>>
>>> I agree. Right now UBIFS uses generate_random_uuid() in both kernel and mkfs.ubifs.
>>> Before we expose it to ->s_uuid, UBIFS should use the correct UUID generation functions.
>>
>> Richard, this makes no sense at all.
>> You have deployed filesystems whose UUID has already been set using
>> generate_random_uuid(). You are not going to reformat those filesystems
>> when generate_random_uuid() dies in the kernel, so how is this even related
>> to exporting the filesystem's UUID to s_uuid?
>
> Well, maybe we need a new feature flag in UBIFS that the UUID is now usable.
> Sure, in the best case we just expose the existing UUID to ->s_uuid and everything
> is fine.
>
>> The changes that Christoph promotes to the kernel uuid common functions
>> are important because we have too many home brewed implementations of
>> uuid handling in the kernel. But making the code cleaner and more reusable
>> isn't going to change the on-disk uuid representation and for filesystems, it
>> probably won't even change the cpu uuid representation (i.e. in super block
>> structs).
>>
>> Seriously, it's not my battle to fight.
>> I don't stand to loose anything if Oleksij's patch gets held hostage
>> for the uuid
>> cleanups, but I don't get the UBIFS maintainer POV on the matter at hand.
>
> My POV is easy, I'm nervous about all this changes and the rush behind them.
> And when Christoph raises concerns, I'm especially careful.
> That's why I'm taking the UBIFS ->s_uuid patch for 4.13 after I had enough
> time to verify and think. And finally that's also why I asked Oleksij
> to follow the discussion. If he can explain in detail why UBIFS does the right
> thing I have a much better feeling. If I have to figure myself it takes time
> which I don't have right now.

Sorry -EIO "why UBIFS does the right thing I have".

If you mean, "will it brake consumers of >s_uuid?". The the answer, if I 
see it correctly, then no.

In my case this patch is needed for EVM and IMA.

linux/security/integrity/evm/evm_crypto.c
...
         if (evm_hmac_attrs & EVM_ATTR_FSUUID)
                 crypto_shash_update(desc, inode->i_sb->s_uuid,
                                     sizeof(inode->i_sb->s_uuid));
...

linux/security/integrity/ima/ima_policy.c
...
         if ((rule->flags & IMA_FSUUID) &&
             memcmp(rule->fsuuid, inode->i_sb->s_uuid, 
sizeof(rule->fsuuid)))
                 return false;
...

Both of them do not use uuid per default and if some one tried to 
configure it for use with UBIFS, then it would just never work as expected.

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Amir Goldstein]

On Tue, May 9, 2017 at 1:09 PM, Richard Weinberger <richard@nod.at> wrote:

>
> Well, maybe we need a new feature flag in UBIFS that the UUID is now usable.

Don't think you will need that, as I wrote, none of the proposed changes have
impact on on-disk uuid format and as Oleksij answered, for EVM/IMA checking
uuid is an opt-in feature.

>
> My POV is easy, I'm nervous about all this changes and the rush behind them.
> And when Christoph raises concerns, I'm especially careful.
> That's why I'm taking the UBIFS ->s_uuid patch for 4.13 after I had enough
> time to verify and think.

Understood your POV :)

Thanks,
Amir.

Re: [PATCH] tmpfs: generate random sb->s_uuid

[Hugh Dickins]

On Tue, 9 May 2017, Amir Goldstein wrote:
> On Tue, May 9, 2017 at 1:09 PM, Richard Weinberger <richard@nod.at> wrote:
> 
> >
> > Well, maybe we need a new feature flag in UBIFS that the UUID is now usable.
> 
> Don't think you will need that, as I wrote, none of the proposed changes have
> impact on on-disk uuid format and as Oleksij answered, for EVM/IMA checking
> uuid is an opt-in feature.
> 
> >
> > My POV is easy, I'm nervous about all this changes and the rush behind them.
> > And when Christoph raises concerns, I'm especially careful.
> > That's why I'm taking the UBIFS ->s_uuid patch for 4.13 after I had enough
> > time to verify and think.
> 
> Understood your POV :)

Thanks a lot for including tmpfs in your enhancements,
and mercifully I don't have to worry much about on-disk compatibility :)
but even so I share Richard's point of view: if Christoph has some
alignment in mind here, then I don't see why the rush to push something
else in right now - and the 0day robot seems to agree that it's rushed.

A welcome addition to tmpfs once fs is agreed on the way to go - thanks.

Hugh

Re: [PATCH] tmpfs: generate random sb->s_uuid

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

Hi Amir,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.11 next-20170505]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/tmpfs-generate-random-sb-s_uuid/20170507-193329
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: mn10300-asb2364_defconfig (attached as .config)
compiler: am33_2.0-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mn10300 

All errors (new ones prefixed by >>):

   mm/shmem.c: In function 'shmem_fill_super':
>> mm/shmem.c:3764:2: error: implicit declaration of function 'generate_random_uuid' [-Werror=implicit-function-declaration]
     generate_random_uuid(sb->s_uuid);
     ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/generate_random_uuid +3764 mm/shmem.c

  3758	#ifdef CONFIG_TMPFS_XATTR
  3759		sb->s_xattr = shmem_xattr_handlers;
  3760	#endif
  3761	#ifdef CONFIG_TMPFS_POSIX_ACL
  3762		sb->s_flags |= MS_POSIXACL;
  3763	#endif
> 3764		generate_random_uuid(sb->s_uuid);
  3765	
  3766		inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0, VM_NORESERVE);
  3767		if (!inode)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 9369 bytes --]