All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tmpfs: generate random sb->s_uuid
@ 2017-05-05  9:44 Amir Goldstein
  2017-05-05 10:01 ` Christoph Hellwig
  2017-05-07 12:11 ` kbuild test robot
  0 siblings, 2 replies; 11+ messages in thread
From: Amir Goldstein @ 2017-05-05  9:44 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Al Viro, linux-unionfs, linux-fsdevel, Hugh Dickins, Andrew Morton

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-05  9:44 [PATCH] tmpfs: generate random sb->s_uuid Amir Goldstein
@ 2017-05-05 10:01 ` Christoph Hellwig
  2017-05-05 10:20   ` Amir Goldstein
  2017-05-07 12:11 ` kbuild test robot
  1 sibling, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2017-05-05 10:01 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Miklos Szeredi, Al Viro, linux-unionfs, linux-fsdevel,
	Hugh Dickins, Andrew Morton

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-05 10:01 ` Christoph Hellwig
@ 2017-05-05 10:20   ` Amir Goldstein
  2017-05-09  9:02     ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2017-05-05 10:20 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Miklos Szeredi, Al Viro, linux-unionfs, linux-fsdevel,
	Hugh Dickins, Andrew Morton

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-05  9:44 [PATCH] tmpfs: generate random sb->s_uuid Amir Goldstein
  2017-05-05 10:01 ` Christoph Hellwig
@ 2017-05-07 12:11 ` kbuild test robot
  1 sibling, 0 replies; 11+ messages in thread
From: kbuild test robot @ 2017-05-07 12:11 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: kbuild-all, Miklos Szeredi, Al Viro, linux-unionfs,
	linux-fsdevel, Hugh Dickins, Andrew Morton

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-05 10:20   ` Amir Goldstein
@ 2017-05-09  9:02     ` Christoph Hellwig
  2017-05-09  9:18       ` Richard Weinberger
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2017-05-09  9:02 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christoph Hellwig, Miklos Szeredi, Al Viro, linux-unionfs,
	linux-fsdevel, Hugh Dickins, Andrew Morton, Richard Weinberger

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-09  9:02     ` Christoph Hellwig
@ 2017-05-09  9:18       ` Richard Weinberger
  2017-05-09  9:54         ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Weinberger @ 2017-05-09  9:18 UTC (permalink / raw)
  To: Christoph Hellwig, Amir Goldstein
  Cc: Miklos Szeredi, Al Viro, linux-unionfs, linux-fsdevel,
	Hugh Dickins, Andrew Morton, Oleksij Rempel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-09  9:18       ` Richard Weinberger
@ 2017-05-09  9:54         ` Amir Goldstein
  2017-05-09 10:09           ` Richard Weinberger
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2017-05-09  9:54 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Christoph Hellwig, Miklos Szeredi, Al Viro, linux-unionfs,
	linux-fsdevel, Hugh Dickins, Andrew Morton, Oleksij Rempel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-09  9:54         ` Amir Goldstein
@ 2017-05-09 10:09           ` Richard Weinberger
  2017-05-09 10:27             ` Oleksij Rempel
  2017-05-09 10:50             ` Amir Goldstein
  0 siblings, 2 replies; 11+ messages in thread
From: Richard Weinberger @ 2017-05-09 10:09 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christoph Hellwig, Miklos Szeredi, Al Viro, linux-unionfs,
	linux-fsdevel, Hugh Dickins, Andrew Morton, Oleksij Rempel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-09 10:09           ` Richard Weinberger
@ 2017-05-09 10:27             ` Oleksij Rempel
  2017-05-09 10:50             ` Amir Goldstein
  1 sibling, 0 replies; 11+ messages in thread
From: Oleksij Rempel @ 2017-05-09 10:27 UTC (permalink / raw)
  To: Richard Weinberger, Amir Goldstein
  Cc: Christoph Hellwig, Miklos Szeredi, Al Viro, linux-unionfs,
	linux-fsdevel, Hugh Dickins, Andrew Morton

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-09 10:09           ` Richard Weinberger
  2017-05-09 10:27             ` Oleksij Rempel
@ 2017-05-09 10:50             ` Amir Goldstein
  2017-05-10  2:51               ` Hugh Dickins
  1 sibling, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2017-05-09 10:50 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Christoph Hellwig, Miklos Szeredi, Al Viro, linux-unionfs,
	linux-fsdevel, Hugh Dickins, Andrew Morton, Oleksij Rempel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tmpfs: generate random sb->s_uuid
  2017-05-09 10:50             ` Amir Goldstein
@ 2017-05-10  2:51               ` Hugh Dickins
  0 siblings, 0 replies; 11+ messages in thread
From: Hugh Dickins @ 2017-05-10  2:51 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Richard Weinberger, Christoph Hellwig, Miklos Szeredi, Al Viro,
	linux-unionfs, linux-fsdevel, Hugh Dickins, Andrew Morton,
	Oleksij Rempel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-05-10  2:51 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-05  9:44 [PATCH] tmpfs: generate random sb->s_uuid Amir Goldstein
2017-05-05 10:01 ` Christoph Hellwig
2017-05-05 10:20   ` Amir Goldstein
2017-05-09  9:02     ` Christoph Hellwig
2017-05-09  9:18       ` Richard Weinberger
2017-05-09  9:54         ` Amir Goldstein
2017-05-09 10:09           ` Richard Weinberger
2017-05-09 10:27             ` Oleksij Rempel
2017-05-09 10:50             ` Amir Goldstein
2017-05-10  2:51               ` Hugh Dickins
2017-05-07 12:11 ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.