diff mbox series

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

Message ID 1611246016-21129-2-git-send-email-dwysocha@redhat.com (mailing list archive)
State New, archived
Headers show
Series Fix crash in trace_rpcgss_context due to 0-length acceptor | expand

Commit Message

David Wysochanski Jan. 21, 2021, 4:20 p.m. UTC 
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(-)

Comments

Trond Myklebust Jan. 21, 2021, 5:05 p.m. UTC | #1 
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)
David Wysochanski Jan. 21, 2021, 5:23 p.m. UTC | #2 
On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust
<trondmy@hammerspace.com> wrote:
>
> 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/ ?
>

Sure.  Do you have any preference for the name?
internal.h
auth_gss.h
something else...


> > +
> >  /*
> >   * 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
>
>
Trond Myklebust Jan. 21, 2021, 6:01 p.m. UTC | #3 
On Thu, 2021-01-21 at 12:23 -0500, David Wysochanski wrote:
> On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > 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/ ?
> > 
> 
> Sure.  Do you have any preference for the name?
> internal.h
> auth_gss.h
> something else...
> 

It is unfortunate that we already have an auth_gss.h in
include/linux/sunrpc. How about auth_gss_internal.h instead?
diff mbox series

Patch

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;
+}
+
+
 /*
  * 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)