Don't initialise new inode generation numbers to zero V2

Don't initialise new inode generation numbers to zero

When we allocation new inode chunks, we initialise the generation numbers to zero. This works fine until we delete a chunk and then reallocate it, resulting in the same inode numbers but with a reset generation count. This can result in inode/generation pairs of different inodes occurring relatively close together.

Given that the inode/gen pair makes up the "unique" portion of an NFS filehandle on XFS, this can result in file handles cached on clients being seen on the wire from the server but refer to a different file. This causes .... issues for NFS clients.

Hence we need a unique generation number initialisation for each inode to prevent reuse of a small portion of the generation number space. Make this initialiser per-allocation group so that it is not a single point of contention in the filesystem, and increment it on every allocation within an AG to reduce the chance that a generation number is reused for a given inode number if the inode chunk is deleted and reallocated immediately afterwards.

Version 2:
o remove persistent per-AGI agi_newinogen field and replace with randomly generated 32 bit number for each new cluster. This prevents NFS clients from potentially guessing what the next generation number is going to be.

Quoted 22 lines · David Chinner

Don't initialise new inode generation numbers to zero

When we allocation new inode chunks, we initialise the generation numbers to zero. This works fine until we delete a chunk and then reallocate it, resulting in the same inode numbers but with a reset generation count. This can result in inode/generation pairs of different inodes occurring relatively close together.

Given that the inode/gen pair makes up the "unique" portion of an NFS filehandle on XFS, this can result in file handles cached on clients being seen on the wire from the server but refer to a different file. This causes .... issues for NFS clients.

Hence we need a unique generation number initialisation for each inode to prevent reuse of a small portion of the generation number space. Make this initialiser per-allocation group so that it is not a single point of contention in the filesystem, and increment it on every allocation within an AG to reduce the chance that a generation number is reused for a given inode number if the inode chunk is deleted and reallocated immediately afterwards.

Quoted 6 lines

Version 2:
o remove persistent per-AGI agi_newinogen field and replace with randomly generated 32 bit number for each new cluster. This prevents NFS clients from potentially guessing what the next generation number is going to be.

I'm confused, why would an NFS client be trying to guess the generation number? AFAICS the important thing is to ensure that the (inode,gen) tuple isn't reused for a long time to prevent accidental filehandle identity issues on clients; whether the gen is predictable or not doesn't matter at all.

Quoted 5 lines

I'm confused, why would an NFS client be trying to guess the generation number? AFAICS the important thing is to ensure that the (inode,gen) tuple isn't reused for a long time to prevent accidental filehandle identity issues on clients; whether the gen is predictable or not doesn't matter at all.

Yeah, that's exactly what I said to Christoph, but that's the issue he raised w.r.t a malicious client triggering inode/gen collisions intentionally. If that's not a problem, then I can just use random32() for the inode number. If it is a real problem, then it needs to be a cryptographically secure random number. Personally, I don't care either way - I just want to get the issue fixed.

Christoph, care to explain how and why this is a problem to everyone?

Quoted 13 lines · David Chinner

Quoted 5 lines

I'm confused, why would an NFS client be trying to guess the generation number? AFAICS the important thing is to ensure that the (inode,gen) tuple isn't reused for a long time to prevent accidental filehandle identity issues on clients; whether the gen is predictable or not doesn't matter at all.

Yeah, that's exactly what I said to Christoph, but that's the issue he raised w.r.t a malicious client triggering inode/gen collisions intentionally. If that's not a problem, then I can just use random32() for the inode number. If it is a real problem, then it needs to be a cryptographically secure random number. Personally, I don't care either way - I just want to get the issue fixed.

Quoted 1 line

Christoph, care to explain how and why this is a problem to everyone?

XFS has some heuristics for inode placement and of course for removing the inode cluster and re-allocting it. I have a gut feeling that there is a small chance to trigger a re-use via nfs operations. Making the initial generation number random means we remove one of the major user-triggerable inputs from the equation.

Ok, so is random32() good enough or does it need to be get_random_int()?

Quoted 1 line · David Chinner

Ok, so is random32() good enough or does it need to be get_random_int()?

I'm not sure offhand what random32 actually is, but a not cryptographically strong random number should be enough.

From lib/random32.c:

Don't initialise new inode generation numbers to zero

When we allocation new inode chunks, we initialise the generation numbers to zero. This works fine until we delete a chunk and then reallocate it, resulting in the same inode numbers but with a reset generation count. This can result in inode/generation pairs of different inodes occurring relatively close together.

Given that the inode/gen pair makes up the "unique" portion of an NFS filehandle on XFS, this can result in file handles cached on clients being seen on the wire from the server but refer to a different file. This causes .... issues for NFS clients.

Hence we need a unique generation number initialisation for each inode to prevent reuse of a small portion of the generation number space. Make this initialiser per-allocation group so that it is not a single point of contention in the filesystem, and increment it on every allocation within an AG to reduce the chance that a generation number is reused for a given inode number if the inode chunk is deleted and reallocated immediately afterwards.

Version 3:
o use random32 rather than get_random_int() as cryptographically secure random numbers are not really necessary here.

Version 2:
o remove persistent per-AGI agi_newinogen field and replace with randomly generated 32 bit number for each new cluster. This prevents NFS clients from potentially guessing what the next generation number is going to be and removes the need for persistent numbers on disk.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_ialloc.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: 2.6.x-xfs-new/fs/xfs/xfs_ialloc.c
===================================================================

fs/xfs/xfs_ialloc.c +9 -0

@@ -147,6 +147,7 @@ xfs_ialloc_ag_alloc(
 	int		version;	/* inode version number to use */
 	int		isaligned = 0;	/* inode allocation at stripe unit */
 					/* boundary */
+	unsigned int	gen;
 
 	args.tp = tp;
 	args.mp = tp->t_mountp;

@@ -290,6 +291,14 @@ xfs_ialloc_ag_alloc(
 	else
 		version = XFS_DINODE_VERSION_1;
 
+	/*
+	 * Seed the new inode cluster with a random generation number. This
+	 * prevents short-term reuse of generation numbers if a chunk is
+	 * freed and then immediately reallocated. We use random numbers
+	 * rather than a linear progression to prevent the next generation
+	 * number from easily guessable.
+	 */
+	gen = random32();
 	for (j = 0; j < nbufs; j++) {
 		/*
 		 * Get the block.

Quoted 3 lines · David Chinner

Christoph, care to explain how and why this is a problem to everyone?

XFS has some heuristics for inode placement and of course for removing the inode cluster and re-allocting it. I have a gut feeling that there is a small chance to trigger a re-use via nfs operations.