[PATCH 3/6] nfsd: uniquify cl_confirm values

[J. Bruce Fields]

On Tue, Nov 27, 2007 at 12:35:04PM -0500, Chuck Lever wrote:
> Hi Bruce-
>
> Some comments below.
>
> On Nov 26, 2007, at 7:32 PM, J. Bruce Fields wrote:
>> Using a counter instead of the nanoseconds value seems more likely to
>> produce a unique cl_confirm.
>>
>> Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
>> ---
>>  fs/nfsd/nfs4state.c |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 035e70a..a26efa8 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -492,14 +492,14 @@ gen_clid(struct nfs4_client *clp) {
>>  }
>>
>>  static void
>> -gen_confirm(struct nfs4_client *clp) {
>> -	struct timespec 	tv;
>> -	u32 *			p;
>> +gen_confirm(struct nfs4_client *clp)
>> +{
>> +	static u32 i = 0;
>> +	u32 *p;
>>
>> -	tv = CURRENT_TIME;
>>  	p = (u32 *)clp->cl_confirm.data;
>> -	*p++ = tv.tv_sec;
>> -	*p++ = tv.tv_nsec;
>> +	*p++ = get_seconds();
>> +	*p++ = i++;
>>  }
>>
>>  static int
>
> get_seconds returns an unsigned long, which could be wider than a u32 on 
> some platforms.  Are you absolutely sure the implicit type conversion is 
> doing what you want here, even on platforms where a long isn't 32 bits?

Whoops, I hadn't noticed that.  But, yes, it's actually fine--since
we're converting to u^32, it'll just reduce mod 2^32, which will cause
collisions only for replays exactly 2^32 seconds (well over a century)
apart.

> I assume that cl_confirm.data is an opaque, so __be32 rather than u32 isn't 
> necessary for the pointer, and endianness is not an issue for the type 
> conversion.

Yes.

> You do need to guarantee that you are getting the bits that 
> change most often into cl_confirm.data.
>
> Also, what kind of serialization do gen_confirm()'s callers use?  Would it 
> be possible for a race to cause two callers to get the same uniquifier?  If 
> it's already safe, it bears adding a comment to that effect.

Alas, practically everything in this file is under one big mutex, so
it's littered with stuff like this.

> A couple of nits: I would avoid using "i" in this case, since you are not 
> indexing something.  "uniq" might be a better name for that variable.

Harrumph.  I thought "i" meant "integer", not "index".  And there's a
special place in my heart for one-letter variable names.

> And, checkpatch.pl (or its maintainer) would probably complain about
> leaving the function's return type declaration on a separate line.

OK, thanks.

>
> Would it be problematic for the confirm ID if time (as returned by 
> get_seconds()) went backward?

Perhaps, if it went backwards far enough that we had time to generate
2^32 cl_confirm's in the time it took to catch up again.

It seems  unlikely enough that I'm not inclined to worry about it.  And
I don't see a simpler way to generate a number guaranteed to be unique
across reboots.  (But I need to think some more about whether that's
actually required.)

> If this value is exposed on the network, it would be better to use a 
> randomized value instead of a time stamp to prevent exposing the system's 
> time on the network (SOP for secure networking).

NFS servers expose the server's time in ever getattr.  (I'm assuming
get_seconds reports gmt or something and not time-since-boot.)

> net_random() is the accepted mechanism to get an adequately random value 
> for such things.  It generates a non-repeating stream of pseudorandom 
> values without a lot of overhead.  You could use the tv_nsec field of 
> CURRENT_TIME as the seed.
>
> Would it be possible for a reboot to cause a replay of the uniquifier 
> values?  If so, is that going to cause trouble?  Instead of starting at 
> zero, the uniquifier should start at a random point.
>
> Starting the uniquifier at a random point would also be wise because 
> otherwise attackers could determine how many of these things have been 
> handed out (not sure if that's actually helpful info, but as mentioned it's 
> best practice to hide any real information if these IDs are going on the 
> network).

The thought of worrying about that just makes me tired.  If you make
patches for the obvious cases (e.g. not lock seqid's where last I
remember it was unclear whether we could get away with non-zero initial
values), I'll take them.

--b.