Re: SUNRPC: Add generic helpers for xdr_stream encode/decode

[Geert Uytterhoeven]

Hi Trond,

On Thu, Mar 2, 2017 at 2:04 AM, Linux Kernel Mailing List
<linux-kernel@xxxxxxxxxxxxxxx> wrote:
> Web:        https://git.kernel.org/torvalds/c/0ae060ca2bdfe11181094699b0f76437ec210b17
> Commit:     0ae060ca2bdfe11181094699b0f76437ec210b17
> Parent:     9761a2469dc287c6d75ca148f4fc483becbcad88
> Refname:    refs/heads/master
> Author:     Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> AuthorDate: Sun Feb 19 16:08:25 2017 -0500
> Committer:  Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
> CommitDate: Tue Feb 21 16:56:16 2017 -0500
>
>     SUNRPC: Add generic helpers for xdr_stream encode/decode
>
>     Add some generic helpers for encoding/decoding opaque structures and
>     basic u32/u64.
>
>     Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>     Reviewed-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>     Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>

> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h

> +/**
> + * xdr_stream_decode_u32 - Decode a 32-bit integer
> + * @xdr: pointer to xdr_stream
> + * @ptr: location to store integer
> + *
> + * Return values:
> + *   %0 on success
> + *   %-EBADMSG on XDR buffer overflow
> + */
> +static inline ssize_t

I think you should either change the return type to "int", or return the
length instead of storing it in *ptr, like all other functions returning
ssize_t do.

The latter will probably kill the (false positive) compiler warning I'm
seeing, too.

> +xdr_stream_decode_u32(struct xdr_stream *xdr, __u32 *ptr)
> +{
> +       const size_t count = sizeof(*ptr);
> +       __be32 *p = xdr_inline_decode(xdr, count);
> +
> +       if (unlikely(!p))
> +               return -EBADMSG;
> +       *ptr = be32_to_cpup(p);
> +       return 0;
> +}
> +
> +/**
> + * xdr_stream_decode_opaque_fixed - Decode fixed length opaque xdr data
> + * @xdr: pointer to xdr_stream
> + * @ptr: location to store data
> + * @len: size of buffer pointed to by @ptr
> + *
> + * Return values:
> + *   On success, returns size of object stored in @ptr
> + *   %-EBADMSG on XDR buffer overflow
> + */
> +static inline ssize_t
> +xdr_stream_decode_opaque_fixed(struct xdr_stream *xdr, void *ptr, size_t len)
> +{
> +       __be32 *p = xdr_inline_decode(xdr, len);
> +
> +       if (unlikely(!p))
> +               return -EBADMSG;
> +       xdr_decode_opaque_fixed(p, ptr, len);
> +       return len;
> +}
> +
> +/**
> + * xdr_stream_decode_opaque_inline - Decode variable length opaque xdr data
> + * @xdr: pointer to xdr_stream
> + * @ptr: location to store pointer to opaque data
> + * @maxlen: maximum acceptable object size
> + *
> + * Note: the pointer stored in @ptr cannot be assumed valid after the XDR
> + * buffer has been destroyed, or even after calling xdr_inline_decode()
> + * on @xdr. It is therefore expected that the object it points to should
> + * be processed immediately.
> + *
> + * Return values:
> + *   On success, returns size of object stored in *@ptr
> + *   %-EBADMSG on XDR buffer overflow
> + *   %-EMSGSIZE if the size of the object would exceed @maxlen
> + */
> +static inline ssize_t
> +xdr_stream_decode_opaque_inline(struct xdr_stream *xdr, void **ptr, size_t maxlen)
> +{
> +       __be32 *p;
> +       __u32 len;
> +
> +       *ptr = NULL;
> +       if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0))
> +               return -EBADMSG;

With gcc-4.1.2, which isn't smart enough to derive the relation between
the initialization of &len and the return value:

    fs/nfs/nfs4xdr.c: In function âdecode_opaque_inlineâ:
    include/linux/sunrpc/xdr.h:409: warning: âlenâ may be used
uninitialized in this function

> +       if (len != 0) {
> +               p = xdr_inline_decode(xdr, len);
> +               if (unlikely(!p))
> +                       return -EBADMSG;
> +               if (unlikely(len > maxlen))
> +                       return -EMSGSIZE;
> +               *ptr = p;
> +       }
> +       return len;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds