Re: [bug report] nfs: Convert to new fscache volume/cookie API

On Fri, Dec 3, 2021 at 11:06 AM David Howells [off-list ref] wrote:

David Wysochanski [off-list ref] wrote:

quoted

quoted

    29         if (*_len > NFS_MAX_KEY_LEN)
    30                 return false;
    31         if (x == 0)
--> 32                 key[(*_len)++] = ',';
    33         else
    34                 *_len += sprintf(key + *_len, ",%llx", x);
    35         return true;
    36 }

This function is very badly broken.  As the checker suggests, the >
should be >= to prevent an array overflow.  But it's actually off by
two because we have to leave space for the NUL terminator so the buffer
is full when "*_len == NFS_MAX_KEY_LEN - 1".  That means the check
should be:

        if (*_len >= NFS_MAX_KEY_LEN - 1)
                return false;

It shouldn't ever overflow the array.  The sprintf really shouldn't insert
more than 18 chars (comma, 16 hex digits and a NUL), but the allocated space
has a 24-char excess to handle that.

Maybe I should use:

        static inline unsigned int how_many_hex_digits(unsigned int x)
        {
                return x ? round_up(ilog2(x) + 1, 4) / 4 : 0;
        }

from fs/cachefiles/key.c to determine how much space I'm actually going to
use.

Actually, I would very much rather not include the comms parameters if I can
avoid it.  They aren't really something that distinguishes volumes on servers
- they're purely a local phenomenon to distinguish local *mounts* made with
different parameters (for which nfs has different superblocks!).

Brainstorming...

1. Use the nfs_server.s_dev (someone can lookup the mount from
/proc/self/mountinfo)
Maybe "%u:%u", MAJOR(sb->s_dev), MINOR(sb->s_dev)

2. Use a checksum on the parameters?

David