Re: [PATCH v2 1/2] SUNRPC: Move simple_get_bytes and simple_get_netobj into xdr.h

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2021-01-21 at 11:20 -0500, Dave Wysochanski wrote:
> Remove duplicated helper functions to parse opaque XDR objects
> and place inside the xdr.h file.  Also update comment which
> is wrong since lockd is not the only user of xdr_netobj.
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  include/linux/sunrpc/xdr.h          | 33
> +++++++++++++++++++++++++++++++--
>  net/sunrpc/auth_gss/auth_gss.c      | 29 ---------------------------
> --
>  net/sunrpc/auth_gss/gss_krb5_mech.c | 29 ---------------------------
> --
>  3 files changed, 31 insertions(+), 60 deletions(-)
> 
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 19b6dea27367..8ef788ff80b9 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -25,8 +25,7 @@
>  #define XDR_QUADLEN(l)         (((l) + 3) >> 2)
>  
>  /*
> - * Generic opaque `network object.' At the kernel level, this type
> - * is used only by lockd.
> + * Generic opaque `network object.'
>   */
>  #define XDR_MAX_NETOBJ         1024
>  struct xdr_netobj {
> @@ -34,6 +33,36 @@ struct xdr_netobj {
>         u8 *                    data;
>  };
>  
> +static inline const void *
> +simple_get_bytes(const void *p, const void *end, void *res, size_t
> len)
> +{
> +       const void *q = (const void *)((const char *)p + len);
> +       if (unlikely(q > end || q < p))
> +               return ERR_PTR(-EFAULT);
> +       memcpy(res, p, len);
> +       return q;
> +}
> +
> +static inline const void *
> +simple_get_netobj(const void *p, const void *end, struct xdr_netobj
> *dest)
> +{
> +       const void *q;
> +       unsigned int len;
> +
> +       p = simple_get_bytes(p, end, &len, sizeof(len));
> +       if (IS_ERR(p))
> +               return p;
> +       q = (const void *)((const char *)p + len);
> +       if (unlikely(q > end || q < p))
> +               return ERR_PTR(-EFAULT);
> +       dest->data = kmemdup(p, len, GFP_NOFS);
> +       if (unlikely(dest->data == NULL))
> +               return ERR_PTR(-ENOMEM);
> +       dest->len = len;
> +       return q;
> +}
> +

Hmm... I'm not too keen on having these in sunrpc/xdr.h. For one thing,
they do not describe objects that are XDR encoded. Secondly, their
naming isn't really consistent with any of the existing conventions for
xdr.

Can we please rather just create a new header file that is private in
net/sunrpc/auth_gss/ ?

> +
>  /*
>   * Basic structure for transmission/reception of a client XDR
> message.
>   * Features a header (for a linear buffer containing RPC headers
> diff --git a/net/sunrpc/auth_gss/auth_gss.c
> b/net/sunrpc/auth_gss/auth_gss.c
> index 4ecc2a959567..228456b22b23 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -125,35 +125,6 @@ struct gss_auth {
>         clear_bit(RPCAUTH_CRED_NEW, &cred->cr_flags);
>  }
>  
> -static const void *
> -simple_get_bytes(const void *p, const void *end, void *res, size_t
> len)
> -{
> -       const void *q = (const void *)((const char *)p + len);
> -       if (unlikely(q > end || q < p))
> -               return ERR_PTR(-EFAULT);
> -       memcpy(res, p, len);
> -       return q;
> -}
> -
> -static inline const void *
> -simple_get_netobj(const void *p, const void *end, struct xdr_netobj
> *dest)
> -{
> -       const void *q;
> -       unsigned int len;
> -
> -       p = simple_get_bytes(p, end, &len, sizeof(len));
> -       if (IS_ERR(p))
> -               return p;
> -       q = (const void *)((const char *)p + len);
> -       if (unlikely(q > end || q < p))
> -               return ERR_PTR(-EFAULT);
> -       dest->data = kmemdup(p, len, GFP_NOFS);
> -       if (unlikely(dest->data == NULL))
> -               return ERR_PTR(-ENOMEM);
> -       dest->len = len;
> -       return q;
> -}
> -
>  static struct gss_cl_ctx *
>  gss_cred_get_ctx(struct rpc_cred *cred)
>  {
> diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c
> b/net/sunrpc/auth_gss/gss_krb5_mech.c
> index ae9acf3a7389..99ea36d5eefe 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_mech.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
> @@ -143,35 +143,6 @@
>         return NULL;
>  }
>  
> -static const void *
> -simple_get_bytes(const void *p, const void *end, void *res, int len)
> -{
> -       const void *q = (const void *)((const char *)p + len);
> -       if (unlikely(q > end || q < p))
> -               return ERR_PTR(-EFAULT);
> -       memcpy(res, p, len);
> -       return q;
> -}
> -
> -static const void *
> -simple_get_netobj(const void *p, const void *end, struct xdr_netobj
> *res)
> -{
> -       const void *q;
> -       unsigned int len;
> -
> -       p = simple_get_bytes(p, end, &len, sizeof(len));
> -       if (IS_ERR(p))
> -               return p;
> -       q = (const void *)((const char *)p + len);
> -       if (unlikely(q > end || q < p))
> -               return ERR_PTR(-EFAULT);
> -       res->data = kmemdup(p, len, GFP_NOFS);
> -       if (unlikely(res->data == NULL))
> -               return ERR_PTR(-ENOMEM);
> -       res->len = len;
> -       return q;
> -}
> -
>  static inline const void *
>  get_key(const void *p, const void *end,
>         struct krb5_ctx *ctx, struct crypto_sync_skcipher **res)

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com