MIME-Version: 1.0
References: <20211201085443.GA24725@kili> <CALF+zOkZgtfP7HrX4oP=qx2uKr3FTRHqECRqKGkRBZaz6F-jdg@mail.gmail.com>
 <997841.1638547576@warthog.procyon.org.uk>
In-Reply-To: <997841.1638547576@warthog.procyon.org.uk>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Fri, 3 Dec 2021 15:05:14 -0500
Message-ID: <CALF+zO=+1JV2JbXWL1zFjkWX=Rz-q93X0C7GTvng77U4LqT91w@mail.gmail.com>
Subject: Re: [bug report] nfs: Convert to new fscache volume/cookie API
To:     David Howells <dhowells@redhat.com>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Dec 3, 2021 at 11:06 AM David Howells <dhowells@redhat.com> wrote:
>
> David Wysochanski <dwysocha@redhat.com> wrote:
>
> > >     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
>