| Message ID | 1493279911-2936-1-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 27, 2017 at 10:58:31AM +0300, Amir Goldstein wrote: > Unless mounted with nouuid, copy the uuid of the filesystem to > struct super block s_uuid field, as several other filesystems do. This looks okay ... ish. Until we have a flag in the superblock that a given fs fills out s_uuid no one can rely on it. So please add the infrastructure that consumers can actually make use of it while you're at it.
On Thu, Apr 27, 2017 at 10:59 AM, Christoph Hellwig <hch@lst.de> wrote: > On Thu, Apr 27, 2017 at 10:58:31AM +0300, Amir Goldstein wrote: >> Unless mounted with nouuid, copy the uuid of the filesystem to >> struct super block s_uuid field, as several other filesystems do. > > This looks okay ... ish. > > Until we have a flag in the superblock that a given fs fills out s_uuid > no one can rely on it. So please add the infrastructure that consumers > can actually make use of it while you're at it. All right. makes sense. will do.
On Thu, Apr 27, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote: > On Thu, Apr 27, 2017 at 10:59 AM, Christoph Hellwig <hch@lst.de> wrote: >> On Thu, Apr 27, 2017 at 10:58:31AM +0300, Amir Goldstein wrote: >>> Unless mounted with nouuid, copy the uuid of the filesystem to >>> struct super block s_uuid field, as several other filesystems do. >> >> This looks okay ... ish. >> >> Until we have a flag in the superblock that a given fs fills out s_uuid >> no one can rely on it. So please add the infrastructure that consumers >> can actually make use of it while you're at it. > > All right. makes sense. will do. Darrick, While I am working on the infrastructure that Christoph requested, please consider this patch as is for now, because: 1. Other filesystems do set s_uuid anyway 2. For my use case, I do check for NULL (zeros) value of s_uuid and this is pretty good validation if UUID has been filled 3. I do anticipate bikeshedding on the proposed infrastructure, mainly because of #2 above Cheers, Amir.
Christoph Hellwig <hch@lst.de> wrote: > Until we have a flag in the superblock that a given fs fills out s_uuid > no one can rely on it. So please add the infrastructure that consumers > can actually make use of it while you're at it. Can't you just look at whether s_uuid is NULL or not? David
On Thu, Apr 27, 2017 at 3:57 PM, David Howells <dhowells@redhat.com> wrote: > Christoph Hellwig <hch@lst.de> wrote: > >> Until we have a flag in the superblock that a given fs fills out s_uuid >> no one can rely on it. So please add the infrastructure that consumers >> can actually make use of it while you're at it. > > Can't you just look at whether s_uuid is NULL or not? > You mean if it is NULL_UUID_LE? Yes, that is what I do now. Christoph suggested that this is not robust enough. I'm rather indifferent about the need of a flag.
Amir Goldstein <amir73il@gmail.com> wrote: > You mean if it is NULL_UUID_LE? Yes, that is what I do now. > Christoph suggested that this is not robust enough. > I'm rather indifferent about the need of a flag. I was thinking more that s_uuid would be a NULL pointer if the fs doesn't have a uuid. David
On 4/27/17 8:22 AM, David Howells wrote: > Amir Goldstein <amir73il@gmail.com> wrote: > >> You mean if it is NULL_UUID_LE? Yes, that is what I do now. >> Christoph suggested that this is not robust enough. >> I'm rather indifferent about the need of a flag. > > I was thinking more that s_uuid would be a NULL pointer if the fs doesn't have > a uuid. but s_uuid in the vfs superblock isn't a pointer: u8 s_uuid[16]; /* UUID */ (or am I missing your point?) -Eric > David
On Thu, Apr 27, 2017 at 4:50 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > > > On 4/27/17 8:22 AM, David Howells wrote: >> Amir Goldstein <amir73il@gmail.com> wrote: >> >>> You mean if it is NULL_UUID_LE? Yes, that is what I do now. >>> Christoph suggested that this is not robust enough. >>> I'm rather indifferent about the need of a flag. >> >> I was thinking more that s_uuid would be a NULL pointer if the fs doesn't have >> a uuid. > > but s_uuid in the vfs superblock isn't a pointer: > > u8 s_uuid[16]; /* UUID */ > > (or am I missing your point?) > > -Eric > IMO, its just too simple to compare to NULL_UUID_LE and/or the set/check a flag. I don't think we should bother with any other option. Amir.
Eric Sandeen <sandeen@sandeen.net> wrote: > but s_uuid in the vfs superblock isn't a pointer: > > u8 s_uuid[16]; /* UUID */ > > (or am I missing your point?) No, I'm getting it wrong. For some reason I'm thinking it's a pointer. I should probably know better. I wonder if it should be a struct uuid_v1... David
On Thu, Apr 27, 2017 at 5:00 PM, David Howells <dhowells@redhat.com> wrote: > Eric Sandeen <sandeen@sandeen.net> wrote: > >> but s_uuid in the vfs superblock isn't a pointer: >> >> u8 s_uuid[16]; /* UUID */ >> >> (or am I missing your point?) > > No, I'm getting it wrong. For some reason I'm thinking it's a pointer. I > should probably know better. I wonder if it should be a struct uuid_v1... > If anything, I think we should hoist uuid_t from fs/xfs/uuid.h. Or arbitrarily use uuid_le, just so it is easy to do uuid_le_cmp(sb->s_uuid, NULL_UUID_LE)
On Thu, Apr 27, 2017 at 10:58:31AM +0300, Amir Goldstein wrote: > Unless mounted with nouuid, copy the uuid of the filesystem to > struct super block s_uuid field, as several other filesystems do. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/xfs/xfs_mount.c | 3 +++ > 1 file changed, 3 insertions(+) > > Darrick, > > The VFS sb->s_uuid field is needed for a new overlay feature > 'constant inode numbers' [1]. > > We store the filesystem uuid along with encoded file handles, so that > we can verify later that we are encoding file handles from the same > filesystem from which the handles were encoded. > > At least the following filesystems set sb->s_uuid: ext4, f2fs, jfs, ocfs2. I dug into the history of s_uuid and discovered that originally it was a way to export the fs uuid via mountinfo in /proc, but that usage went away quickly, so now the only users are cleancache, evm, and ima. The last two will use s_uuid if the administrator (I think?) tells it to, and it doesn't seem to care about the uniqueness. To me that sounds like the semantics of s_uuid are to fill it in if the fs wants to, but that the fs doesn't make any guarantees about the contents. > Specifically, btrfs does not set sb->s_uuid, I think because it has many > uuid's per super_block struct. > > I see no obvious reason for xfs not to set sb->s_uuid so here goes. > > I made a choice not to set sb->s_uuid in case xfs was mounted with nouuid, > to maintain a self inflicted rule that sb->s_uuid is unique in a system > for an xfs super_block. This is an arbitrary decision so others may not > agree with it. > > My reasoning in the context of verifying file handles is this - > If a file handle was exported from one copy of an xfs filesystem, I rather > it was not decoded from another copy of the filesystem (i.e. LVM snapshot), > at least not while both copies are mounted on the same system. I disagree. ext4, f2fs, gfs2, ocfs2, and ubifs all set s_uuid unconditionally, even if that means there are multiple struct superblocks floating around with the same s_uuid. Now, I surmise that for overlayfs copy-up you want to be able to store (sb_uuid, fh) as an xattr to keep track of the original inode, and for that you really /do/ want sb_uuid to be unique. But therein lies a problem -- if another ext4 fs shows up with the same uuid and the overlayfs accidentally gets paired with the second ext4, your stored xattr is toast because you can't tell that this is the wrong filesystem... unless you already detect this situation? I suppose since this xattr thing is an optimization(?) you /could/ actually keep track of whether or not there are mounted fses with the same s_uuid and therefore know whether or not it's safe to use it. I don't have an objection to XFS filling out s_uuid unconditionally like the other filesystems do. If we want to change the meaning of that field, let's change all of the fses at once. > I tested the patch is working correctly with and without nouuid with my > overlayfs constant inode tests. I do have xfstests that check overlay > constant inodes, but they are of little use to you without the overlayfs > patches. > > Here is what it looks like when running constant inode verification test > for overlayfs above xfs mounted with nouuid: > ~/unionmount-testsuite# ./run --ov=0 --samefs hard-link > ... > XFS (vdf): Ending clean mount > overlayfs: lower fs needs to report s_uuid. > ./run --link /mnt/a/foo100 /mnt/a/no_foo100 > sh (2748): drop_caches: 3 > overlayfs: lower fs needs to report s_uuid. > /mnt/a/no_foo100: inode number wrong (got 442, want 137) > > The same test passes with overlayfs over ext4 and with overlay over xfs What happens if you create two ext4 filesystems with the same uuid, mount an overlay with one ext4, make some changes, then unmount the overlay and mount it with the other ext4? --D > mounted without nouuid (and with this patch applied naturally). > > I'd appreciate if you could queue this simple patch for v4.12. > > Thanks, > Amir. > > [1] https://marc.info/?l=linux-unionfs&m=149324252301397&w=2 > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 450bde6..29e45a0 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -100,6 +100,9 @@ xfs_uuid_mount( > xfs_uuid_table[hole] = *uuid; > mutex_unlock(&xfs_uuid_table_mutex); > > + /* 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)); > return 0; > > out_duplicate: > -- > 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
On 4/27/17 2:31 PM, Darrick J. Wong wrote: > On Thu, Apr 27, 2017 at 10:58:31AM +0300, Amir Goldstein wrote: ... >> I made a choice not to set sb->s_uuid in case xfs was mounted with nouuid, >> to maintain a self inflicted rule that sb->s_uuid is unique in a system >> for an xfs super_block. This is an arbitrary decision so others may not >> agree with it. >> >> My reasoning in the context of verifying file handles is this - >> If a file handle was exported from one copy of an xfs filesystem, I rather >> it was not decoded from another copy of the filesystem (i.e. LVM snapshot), >> at least not while both copies are mounted on the same system. > > I disagree. ext4, f2fs, gfs2, ocfs2, and ubifs all set s_uuid > unconditionally, even if that means there are multiple struct > superblocks floating around with the same s_uuid. Now, I surmise that > for overlayfs copy-up you want to be able to store (sb_uuid, fh) as an > xattr to keep track of the original inode, and for that you really /do/ > want sb_uuid to be unique. > > But therein lies a problem -- if another ext4 fs shows up with the same > uuid and the overlayfs accidentally gets paired with the second ext4, > your stored xattr is toast because you can't tell that this is the wrong > filesystem... unless you already detect this situation? > > I suppose since this xattr thing is an optimization(?) you /could/ > actually keep track of whether or not there are mounted fses with the > same s_uuid and therefore know whether or not it's safe to use it. > > I don't have an objection to XFS filling out s_uuid unconditionally like > the other filesystems do. If we want to change the meaning of that > field, let's change all of the fses at once. So this all reminded me that I had a rework of xfs's unique UUID detection routine to iterate all the xfs supers in the system looking for a duplicate. (vs our current homegrown table mess). It'd be trivial to make that a vfs function and set a flag on the vfs super if the UUID is not unique. Would that be worthwhile? I can send a patch if so. -Eric
On Thu, Apr 27, 2017 at 03:30:35PM -0500, Eric Sandeen wrote: > It'd be trivial to make that a vfs function and set a flag on the vfs super > if the UUID is not unique. Would that be worthwhile? I can send a patch > if so. This only tells you that the UUID it has is unique now. As you are contemplating storing them in the FS they have to be unique in space _and_ time. No check you can do now can tell you if the UUID is unique for all time. Right ? -apw
On Fri, Apr 28, 2017 at 06:49:45AM +0100, Andy Whitcroft wrote: > On Thu, Apr 27, 2017 at 03:30:35PM -0500, Eric Sandeen wrote: > > > It'd be trivial to make that a vfs function and set a flag on the vfs super > > if the UUID is not unique. Would that be worthwhile? I can send a patch > > if so. > > This only tells you that the UUID it has is unique now. As you are > contemplating storing them in the FS they have to be unique in space > _and_ time. No check you can do now can tell you if the UUID is unique > for all time. Right ? Well, since we're talking about all time: "probably" :P That said, Eric was talking about a refactoring of one of XFS's mount time checks that errors out if there's already a mounted fs with the same uuid. We use it to avoid catastrophic mounting of the same fs from multiple (misconfigured) multipaths/snapshots/volumes/whatever on the same machine. Not sure if that's useful for Amir's case. --D > > -apw > -- > 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
On Thu, Apr 27, 2017 at 10:31 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Thu, Apr 27, 2017 at 10:58:31AM +0300, Amir Goldstein wrote: >> Unless mounted with nouuid, copy the uuid of the filesystem to >> struct super block s_uuid field, as several other filesystems do. >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> fs/xfs/xfs_mount.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> Darrick, >> >> The VFS sb->s_uuid field is needed for a new overlay feature >> 'constant inode numbers' [1]. >> >> We store the filesystem uuid along with encoded file handles, so that >> we can verify later that we are encoding file handles from the same >> filesystem from which the handles were encoded. >> [...] >> >> I made a choice not to set sb->s_uuid in case xfs was mounted with nouuid, >> to maintain a self inflicted rule that sb->s_uuid is unique in a system >> for an xfs super_block. This is an arbitrary decision so others may not >> agree with it. >> >> My reasoning in the context of verifying file handles is this - >> If a file handle was exported from one copy of an xfs filesystem, I rather >> it was not decoded from another copy of the filesystem (i.e. LVM snapshot), >> at least not while both copies are mounted on the same system. > > I disagree. ext4, f2fs, gfs2, ocfs2, and ubifs all set s_uuid > unconditionally, even if that means there are multiple struct > superblocks floating around with the same s_uuid. Now, I surmise that > for overlayfs copy-up you want to be able to store (sb_uuid, fh) as an > xattr to keep track of the original inode, and for that you really /do/ > want sb_uuid to be unique. > No. I don't care about sb_uuid being unique. As I wrote I am indifferent to the choice of exporting with -o nouuid. Your reasoning works for me (do what everyone else does), so I'll change the patch and re-post. > But therein lies a problem -- if another ext4 fs shows up with the same > uuid and the overlayfs accidentally gets paired with the second ext4, > your stored xattr is toast because you can't tell that this is the wrong > filesystem... unless you already detect this situation? > No. I don't bother. > I suppose since this xattr thing is an optimization(?) you /could/ > actually keep track of whether or not there are mounted fses with the > same s_uuid and therefore know whether or not it's safe to use it. > It is not only an optimization. It's partly a sanity check to prevent admin from making mistakes, partly a way to automatically detect that overlay layers were copied over so file handles should not be trusted. It is certainly not going to prevent an admin that wants to shoot himself in the leg carry out his plans. > I don't have an objection to XFS filling out s_uuid unconditionally like > the other filesystems do. If we want to change the meaning of that > field, let's change all of the fses at once. > I am happy with the way things are... [...] > > What happens if you create two ext4 filesystems with the same uuid, > mount an overlay with one ext4, make some changes, then unmount the > overlay and mount it with the other ext4? > As I wrote, if admin wants to shoot himself in the leg, he is free to do so just as he is with NFS export. In the example you gave, admin could re-export the same NFS share from the other ext4 fs (and nfsd doesn't even check UUID AFAIK), so NFS client coming with old file handles could be served with completely different files from the new fs. I do want to protect admin from things that could go wrong with normal behavior: - copying layers on the same fs and mounting overlay - verify file handles (*) - copying layers to a different fs and mounting overlay - verify uuid - copying an entire fs (or LVM snapshot) and mount an 'overlay clone' - it's ok to use the handles in the clone even if they point to files that diverged from the original fs. It's just as if the overlay mount content itself was cloned and diverged from its original copy. (*) on the samefs case, file handles could still be decoded and point to a different lower layer, but that is a calculated risk we are willing to live with, because the file handles are just used to find a unique inode number and pin the inode while this inode number is exposed Amir.
On Fri, Apr 28, 2017 at 8:56 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Fri, Apr 28, 2017 at 06:49:45AM +0100, Andy Whitcroft wrote: >> On Thu, Apr 27, 2017 at 03:30:35PM -0500, Eric Sandeen wrote: >> >> > It'd be trivial to make that a vfs function and set a flag on the vfs super >> > if the UUID is not unique. Would that be worthwhile? I can send a patch >> > if so. >> >> This only tells you that the UUID it has is unique now. As you are >> contemplating storing them in the FS they have to be unique in space >> _and_ time. No check you can do now can tell you if the UUID is unique >> for all time. Right ? > > Well, since we're talking about all time: "probably" :P > > That said, Eric was talking about a refactoring of one of XFS's mount > time checks that errors out if there's already a mounted fs with the > same uuid. We use it to avoid catastrophic mounting of the same fs from > multiple (misconfigured) multipaths/snapshots/volumes/whatever on the > same machine. Not sure if that's useful for Amir's case. > As far as I can tell, we don't mind that sb->s_uuid is not unique. All we care about is to verify that the file handle we are holding was encoded from the same file system instance OR from one of its derivative (LVM snapshot etc), just as a sanity that we won't get a completely different object (from a different fs type even) by mistake. If admin zygotes all his ext4 fs with the same uuid and fill them with different content and copies overlay layers between them.... Let me know if you think there is a real use case for this so I can sympathize. Amir.
On Fri, Apr 28, 2017 at 8:18 AM, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Apr 28, 2017 at 8:56 AM, Darrick J. Wong > <darrick.wong@oracle.com> wrote: >> On Fri, Apr 28, 2017 at 06:49:45AM +0100, Andy Whitcroft wrote: >>> On Thu, Apr 27, 2017 at 03:30:35PM -0500, Eric Sandeen wrote: >>> >>> > It'd be trivial to make that a vfs function and set a flag on the vfs super >>> > if the UUID is not unique. Would that be worthwhile? I can send a patch >>> > if so. >>> >>> This only tells you that the UUID it has is unique now. As you are >>> contemplating storing them in the FS they have to be unique in space >>> _and_ time. No check you can do now can tell you if the UUID is unique >>> for all time. Right ? I don't understand this fuss about checking if a unique identifier is unique. It obviously is, if not don't call it uuid, call it nuuid. Thanks, Miklos
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 450bde6..29e45a0 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -100,6 +100,9 @@ xfs_uuid_mount( xfs_uuid_table[hole] = *uuid; mutex_unlock(&xfs_uuid_table_mutex); + /* 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)); return 0; out_duplicate:
Unless mounted with nouuid, copy the uuid of the filesystem to struct super block s_uuid field, as several other filesystems do. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/xfs/xfs_mount.c | 3 +++ 1 file changed, 3 insertions(+) Darrick, The VFS sb->s_uuid field is needed for a new overlay feature 'constant inode numbers' [1]. We store the filesystem uuid along with encoded file handles, so that we can verify later that we are encoding file handles from the same filesystem from which the handles were encoded. At least the following filesystems set sb->s_uuid: ext4, f2fs, jfs, ocfs2. Specifically, btrfs does not set sb->s_uuid, I think because it has many uuid's per super_block struct. I see no obvious reason for xfs not to set sb->s_uuid so here goes. I made a choice not to set sb->s_uuid in case xfs was mounted with nouuid, to maintain a self inflicted rule that sb->s_uuid is unique in a system for an xfs super_block. This is an arbitrary decision so others may not agree with it. My reasoning in the context of verifying file handles is this - If a file handle was exported from one copy of an xfs filesystem, I rather it was not decoded from another copy of the filesystem (i.e. LVM snapshot), at least not while both copies are mounted on the same system. I tested the patch is working correctly with and without nouuid with my overlayfs constant inode tests. I do have xfstests that check overlay constant inodes, but they are of little use to you without the overlayfs patches. Here is what it looks like when running constant inode verification test for overlayfs above xfs mounted with nouuid: ~/unionmount-testsuite# ./run --ov=0 --samefs hard-link ... XFS (vdf): Ending clean mount overlayfs: lower fs needs to report s_uuid. ./run --link /mnt/a/foo100 /mnt/a/no_foo100 sh (2748): drop_caches: 3 overlayfs: lower fs needs to report s_uuid. /mnt/a/no_foo100: inode number wrong (got 442, want 137) The same test passes with overlayfs over ext4 and with overlay over xfs mounted without nouuid (and with this patch applied naturally). I'd appreciate if you could queue this simple patch for v4.12. Thanks, Amir. [1] https://marc.info/?l=linux-unionfs&m=149324252301397&w=2