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

On Thu, 2021-01-21 at 12:23 -0500, David Wysochanski wrote:

On Thu, Jan 21, 2021 at 12:05 PM Trond Myklebust
[off-list ref] wrote:

quoted

On Thu, 2021-01-21 at 11:20 -0500, Dave Wysochanski wrote:

quoted

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 <redacted>
---
 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?

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