[PATCH v2] xfs: publish UUID in struct super_block

[PATCH v2] xfs: publish UUID in struct super_block

[Amir Goldstein]

Copy the uuid of the filesystem to struct super_block s_uuid field,
as several other filesystems already do.  Copy regardless of the nouuid
mount option, because other filesystems also do not guaranty uniqueness
of the s_uuid field in super_block struct.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_mount.c | 4 ++++
 1 file changed, 4 insertions(+)

Darrick,

Per your request I moved the copy above nouuid check.

Cheers,
Amir.

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 450bde6..4c0d8d7 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -73,6 +73,10 @@ xfs_uuid_mount(
 	uuid_t			*uuid = &mp->m_sb.sb_uuid;
 	int			hole, i;
 
+	/* Publish UUID in struct super_block */
+	BUILD_BUG_ON(sizeof(mp->m_super->s_uuid) != sizeof(uuid_t));
+	memcpy(&mp->m_super->s_uuid, uuid, sizeof(uuid_t));
+
 	if (mp->m_flags & XFS_MOUNT_NOUUID)
 		return 0;
 
-- 
2.7.4

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Darrick J. Wong]

On Fri, Apr 28, 2017 at 05:00:01PM +0300, Amir Goldstein wrote:
> Copy the uuid of the filesystem to struct super_block s_uuid field,
> as several other filesystems already do.  Copy regardless of the nouuid
> mount option, because other filesystems also do not guaranty uniqueness
> of the s_uuid field in super_block struct.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_mount.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> Darrick,
> 
> Per your request I moved the copy above nouuid check.
> 
> Cheers,
> Amir.
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 450bde6..4c0d8d7 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -73,6 +73,10 @@ xfs_uuid_mount(
>  	uuid_t			*uuid = &mp->m_sb.sb_uuid;
>  	int			hole, i;
>  
> +	/* Publish UUID in struct super_block */
> +	BUILD_BUG_ON(sizeof(mp->m_super->s_uuid) != sizeof(uuid_t));
> +	memcpy(&mp->m_super->s_uuid, uuid, sizeof(uuid_t));
> +
>  	if (mp->m_flags & XFS_MOUNT_NOUUID)
>  		return 0;
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Christoph Hellwig]

On Fri, Apr 28, 2017 at 05:00:01PM +0300, Amir Goldstein wrote:
> Copy the uuid of the filesystem to struct super_block s_uuid field,
> as several other filesystems already do.  Copy regardless of the nouuid
> mount option, because other filesystems also do not guaranty uniqueness
> of the s_uuid field in super_block struct.

No guaranteeing uniqueness will create problems, don't do that.

Other file system didn't use to do the uuid table check that XFS did
either, and that's a fatal bug.  In the long run we'll need to move
this check to the VFS now that we have s_uuid.

Also while checking for a nul uuid is probably ok we need to formalize
that at least that the check is needed.  Preferably by adding a little
inline helper for it, and documenting it.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Amir Goldstein]

On Tue, May 2, 2017 at 10:30 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Fri, Apr 28, 2017 at 05:00:01PM +0300, Amir Goldstein wrote:
>> Copy the uuid of the filesystem to struct super_block s_uuid field,
>> as several other filesystems already do.  Copy regardless of the nouuid
>> mount option, because other filesystems also do not guaranty uniqueness
>> of the s_uuid field in super_block struct.
>
> No guaranteeing uniqueness will create problems, don't do that.

How can it create problems if uniqueness is not guaranteed with
Current s_uuid? Even if we did make the xfs uuid table code generic
It couldn't be the vfs default. Filesystems will have to opt in.

>
> Other file system didn't use to do the uuid table check that XFS did
> either, and that's a fatal bug.  In the long run we'll need to move
> this check to the VFS now that we have s_uuid.
>
> Also while checking for a nul uuid is probably ok we need to formalize
> that at least that the check is needed.  Preferably by adding a little
> inline helper for it, and documenting it.

Makes sense. I'll post a patch.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Christoph Hellwig]

On Tue, May 02, 2017 at 05:13:56PM +0300, Amir Goldstein wrote:
> How can it create problems if uniqueness is not guaranteed with
> Current s_uuid? Even if we did make the xfs uuid table code generic
> It couldn't be the vfs default. Filesystems will have to opt in.

It creates problems if you e.g. mount an ext4 fs and a dm snaphot of
it.  The non-XFS file systems are simply buggy in this regard.

Non-uniqueue uuids are an absolute no-go.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Amir Goldstein]

On Tue, May 2, 2017 at 5:17 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, May 02, 2017 at 05:13:56PM +0300, Amir Goldstein wrote:
>> How can it create problems if uniqueness is not guaranteed with
>> Current s_uuid? Even if we did make the xfs uuid table code generic
>> It couldn't be the vfs default. Filesystems will have to opt in.
>
> It creates problems if you e.g. mount an ext4 fs and a dm snaphot of
> it.  The non-XFS file systems are simply buggy in this regard.
>
> Non-uniqueue uuids are an absolute no-go.

I'm not sure I follow your specific concern here.
Surely you are not proposing to get rid of the nouuid
mount option, are you? So what's the point of hiding
the fact that there are 2 mounted filesystems with the
same uuid from VFS?

Because that is the the only implication of exporting
s_uuid regardless of nouuid mount option.

Whether or not ext4 and other fs should restrict
multi mount of same uuid is a completely different
issue.

Amir.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Christoph Hellwig]

On Tue, May 02, 2017 at 05:27:36PM +0300, Amir Goldstein wrote:
> On Tue, May 2, 2017 at 5:17 PM, Christoph Hellwig <hch@lst.de> wrote:
> > On Tue, May 02, 2017 at 05:13:56PM +0300, Amir Goldstein wrote:
> >> How can it create problems if uniqueness is not guaranteed with
> >> Current s_uuid? Even if we did make the xfs uuid table code generic
> >> It couldn't be the vfs default. Filesystems will have to opt in.
> >
> > It creates problems if you e.g. mount an ext4 fs and a dm snaphot of
> > it.  The non-XFS file systems are simply buggy in this regard.
> >
> > Non-uniqueue uuids are an absolute no-go.
> 
> I'm not sure I follow your specific concern here.
> Surely you are not proposing to get rid of the nouuid
> mount option, are you? So what's the point of hiding
> the fact that there are 2 mounted filesystems with the
> same uuid from VFS?

Because it breaks people using s_uuid.  Take a look at cleancache,
which identifies a pool with it.  Once you have to snapshot with
the same uuid the pool concept is broken.  Same for any sort of
use in file handles.

The U in UUID stands for unique, and we must ensure that's actually
true.

> 
> Because that is the the only implication of exporting
> s_uuid regardless of nouuid mount option.
> 
> Whether or not ext4 and other fs should restrict
> multi mount of same uuid is a completely different
> issue.

It's not.  The whole point of exporting s_uuid is to have a uniqueue
identifier for a superblock.  If it's not actually uniqueue there is
no point in having or using it.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Miklos Szeredi]

On Tue, May 2, 2017 at 4:47 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, May 02, 2017 at 05:27:36PM +0300, Amir Goldstein wrote:
>> On Tue, May 2, 2017 at 5:17 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > On Tue, May 02, 2017 at 05:13:56PM +0300, Amir Goldstein wrote:
>> >> How can it create problems if uniqueness is not guaranteed with
>> >> Current s_uuid? Even if we did make the xfs uuid table code generic
>> >> It couldn't be the vfs default. Filesystems will have to opt in.
>> >
>> > It creates problems if you e.g. mount an ext4 fs and a dm snaphot of
>> > it.  The non-XFS file systems are simply buggy in this regard.
>> >
>> > Non-uniqueue uuids are an absolute no-go.
>>
>> I'm not sure I follow your specific concern here.
>> Surely you are not proposing to get rid of the nouuid
>> mount option, are you? So what's the point of hiding
>> the fact that there are 2 mounted filesystems with the
>> same uuid from VFS?
>
> Because it breaks people using s_uuid.  Take a look at cleancache,
> which identifies a pool with it.  Once you have to snapshot with
> the same uuid the pool concept is broken.  Same for any sort of
> use in file handles.
>
> The U in UUID stands for unique, and we must ensure that's actually
> true.

Two separate issues:

 a) is s_uuid unique across all currently mounted filesystems on this system
 b) is s_uuid unique for all different filesystems that have been
mounted at one time on some system

We can check (a) but not (b).  But failing (b) could have equally bad
consequences as failing (a).

Thanks,
Miklos

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Christoph Hellwig]

On Tue, May 02, 2017 at 05:08:26PM +0200, Miklos Szeredi wrote:
> Two separate issues:
> 
>  a) is s_uuid unique across all currently mounted filesystems on this system
>  b) is s_uuid unique for all different filesystems that have been
> mounted at one time on some system
> 
> We can check (a) but not (b).  But failing (b) could have equally bad
> consequences as failing (a).

While (b) is harmful it's not anywhere near as harmful as (a).  And
while we can trivially protect against (a) protecting against the full
scope of (b) (e.g. including reboots) would be very hard.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Amir Goldstein]

On Tue, May 2, 2017 at 5:47 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, May 02, 2017 at 05:27:36PM +0300, Amir Goldstein wrote:
>> On Tue, May 2, 2017 at 5:17 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > On Tue, May 02, 2017 at 05:13:56PM +0300, Amir Goldstein wrote:
>> >> How can it create problems if uniqueness is not guaranteed with
>> >> Current s_uuid? Even if we did make the xfs uuid table code generic
>> >> It couldn't be the vfs default. Filesystems will have to opt in.
>> >
>> > It creates problems if you e.g. mount an ext4 fs and a dm snaphot of
>> > it.  The non-XFS file systems are simply buggy in this regard.
>> >
>> > Non-uniqueue uuids are an absolute no-go.
>>
>> I'm not sure I follow your specific concern here.
>> Surely you are not proposing to get rid of the nouuid
>> mount option, are you? So what's the point of hiding
>> the fact that there are 2 mounted filesystems with the
>> same uuid from VFS?
>
> Because it breaks people using s_uuid.

Christoph,

I do agree with you that that situation where s_uuid is unique on a system
is preferred over what we have today. I disagree that not respecting
uniqueness that was not respected until now has the potential to
break any user.

New users that require uniqueness from s_uuid, those are the ones that
should be checking for the not yet existing flag SB_I_AM_UNIQUE.
Old users cannot claim that uniqueness has been broken.
Whether or not there are users that need to check SB_I_AM_UNIQUE
and whether or not we need to hoist the xfs unique uuid implementation
to VFS remains to be seen.

> Take a look at cleancache,
> which identifies a pool with it.  Once you have to snapshot with
> the same uuid the pool concept is broken.  Same for any sort of
> use in file handles.
>

I am not sure that cleancache is a good example for your argument.
s_uuid is needed for the cleancache_init_shared_fs() case, only used by
ocfs2 in-tree fs. From what I can gather from cleancache documentation,
this is meant for a use case where 2 different VMs use the same shared
pool. If I understood correctly, then making s_uuid unique on a single
system is not good enough for this use case anyway.

> The U in UUID stands for unique, and we must ensure that's actually
> true.

Maybe we should re-interpret the U of Universal as Utopic ;-)

>
>>
>> Because that is the the only implication of exporting
>> s_uuid regardless of nouuid mount option.
>>
>> Whether or not ext4 and other fs should restrict
>> multi mount of same uuid is a completely different
>> issue.
>
> It's not.  The whole point of exporting s_uuid is to have a uniqueue
> identifier for a superblock.  If it's not actually uniqueue there is
> no point in having or using it.

Well it's useful for my use case. I need s_uuid as a sanity check
and IIUC, so does cleancache.
I don't know about EVM/IMA, but apparently, it did well
with pseudo uniqueness so far.

Cheers,
Amir.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Amir Goldstein]

On Tue, May 2, 2017 at 8:09 PM, Amir Goldstein <amir73il@gmail.com> wrote:
...
>>> > Non-uniqueue uuids are an absolute no-go.
>>>
>>> I'm not sure I follow your specific concern here.
>>> Surely you are not proposing to get rid of the nouuid
>>> mount option, are you? So what's the point of hiding
>>> the fact that there are 2 mounted filesystems with the
>>> same uuid from VFS?
>>
>> Because it breaks people using s_uuid.
...

>>
>> It's not.  The whole point of exporting s_uuid is to have a uniqueue
>> identifier for a superblock.  If it's not actually uniqueue there is
>> no point in having or using it.
>
> Well it's useful for my use case. I need s_uuid as a sanity check
> and IIUC, so does cleancache.
> I don't know about EVM/IMA, but apparently, it did well
> with pseudo uniqueness so far.
>

Mimi,

Can you please comment on the implication of starting to publish
sb->s_uuid by xfs on EVM/IMA.

Are there any persistent integrity signatures that could be broken,
because an xfs filesystem once published zeroes for sb->s_uuid
and will start to publish non-zero sb->s_uuid?

BTW, the same question applies w.r.t ubifs, which are also going to
start publishing sb->s_uuid, although I don't if there are ubifs+evm users.

Thanks,
Amir.

Re: [PATCH v2] xfs: publish UUID in struct super_block

[Richard Weinberger]

Amir,

Am 02.05.2017 um 20:25 schrieb Amir Goldstein:
> Are there any persistent integrity signatures that could be broken,
> because an xfs filesystem once published zeroes for sb->s_uuid
> and will start to publish non-zero sb->s_uuid?
> 
> BTW, the same question applies w.r.t ubifs, which are also going to
> start publishing sb->s_uuid, although I don't if there are ubifs+evm users.

There are. And I'm very sure this is why Oleksij is trying to mainline
UBIFS sb->s_uuid support.

Thanks,
//richard