Re: [PATCH] Convert gre_sysctl to sysctl_bounded_arr

[George Koehler]

ok gkoehler@ with one comment change, see below.

On Sat, 21 Nov 2020 15:36:58 -0800
Greg Steuck  wrote:

> diff --git sys/net/if_gre.c sys/net/if_gre.c
> index cb5beeaad62..deacad9c755 100644
> --- sys/net/if_gre.c
> +++ sys/net/if_gre.c
> @@ -1109,7 +1109,7 @@ gre_input_key(struct mbuf **mp, int *offp, int type, 
> int af, uint8_t otos,
>   if (n == NULL)
>   goto decline;
>   if (n->m_data[off] >> 4 != IPVERSION)
> - hlen += sizeof(gre_wccp);
> + hlen += 4;  /* four-octet GRE header */
>  
>   /* FALLTHROUGH */
>   }

I want the comment to be, "four-octet Redirect header".  This is my
best guess after I glanced at section 4.12.1 of
https://tools.ietf.org/html/draft-wilson-wrec-wccp-v2-00

The Redirect header would be after the GRE header; the sizeof(*gh) in
"hlen = iphlen + sizeof(*gh);" was the GRE header.  I agree that using
sizeof(gre_wccp) is wrong, because that's the size of the
sysctl net.inet.gre.wccp, not the size of any header.

I don't have any way to test the wccp code.  --George

> @@ -4229,31 +4229,22 @@ drop:
>   return (NULL);
>  }
>  
> +const struct sysctl_bounded_args gre_vars[] = {
> + { GRECTL_ALLOW, _allow, 0, 1 },
> + { GRECTL_WCCP, _wccp, 0, 1 },
> +};
> +
>  int
>  gre_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
>  size_t newlen)
>  {
>   int error;
>  
> - /* All sysctl names at this level are terminal. */
> - if (namelen != 1)
> - return (ENOTDIR);
> -
> - switch (name[0]) {
> - case GRECTL_ALLOW:
> - NET_LOCK();
> - error = sysctl_int(oldp, oldlenp, newp, newlen, _allow);
> - NET_UNLOCK();
> - return (error);
> - case GRECTL_WCCP:
> - NET_LOCK();
> - error = sysctl_int(oldp, oldlenp, newp, newlen, _wccp);
> - NET_UNLOCK();
> - return (error);
> - default:
> - return (ENOPROTOOPT);
> - }
> - /* NOTREACHED */
> + NET_LOCK();
> + error = sysctl_bounded_arr(gre_vars, nitems(gre_vars), name,
> + namelen, oldp, oldlenp, newp, newlen);
> + NET_UNLOCK();
> + return error;
>  }
>  
>  static inline int
> -- 
> 2.29.2
> 


-- 
George Koehler 

[PATCH] Convert gre_sysctl to sysctl_bounded_arr

[Greg Steuck]

Hi David,

What do you think about this patch? I suspect the reference to the
variable is accidental. I changed it to a hardcoded 4 with a quote from
draft-wilson-wrec-wccp-v2-01, though I'm not entirely sure I found the
right reference.

And since I hope to get your attention, maybe you can also OK the
sysctl conversion? Tell me if one or two commits is preferable.

Thanks
Greg

>From 0759b51ee5c612d3f922a334f5eddf2bc1fa053a Mon Sep 17 00:00:00 2001
From: Greg Steuck 
Date: Sat, 21 Nov 2020 14:48:12 -0800
Subject: [PATCH] Convert gre_sysctl to sysctl_bounded_arr

Fixed up a reference to gre_wccp where a fixed value from wwcp
standard was intended.
---
 sys/net/if_gre.c | 31 +++
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git sys/net/if_gre.c sys/net/if_gre.c
index cb5beeaad62..deacad9c755 100644
--- sys/net/if_gre.c
+++ sys/net/if_gre.c
@@ -1109,7 +1109,7 @@ gre_input_key(struct mbuf **mp, int *offp, int type, int 
af, uint8_t otos,
if (n == NULL)
goto decline;
if (n->m_data[off] >> 4 != IPVERSION)
-   hlen += sizeof(gre_wccp);
+   hlen += 4;  /* four-octet GRE header */
 
/* FALLTHROUGH */
}
@@ -4229,31 +4229,22 @@ drop:
return (NULL);
 }
 
+const struct sysctl_bounded_args gre_vars[] = {
+   { GRECTL_ALLOW, _allow, 0, 1 },
+   { GRECTL_WCCP, _wccp, 0, 1 },
+};
+
 int
 gre_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
 size_t newlen)
 {
int error;
 
-   /* All sysctl names at this level are terminal. */
-   if (namelen != 1)
-   return (ENOTDIR);
-
-   switch (name[0]) {
-   case GRECTL_ALLOW:
-   NET_LOCK();
-   error = sysctl_int(oldp, oldlenp, newp, newlen, _allow);
-   NET_UNLOCK();
-   return (error);
-   case GRECTL_WCCP:
-   NET_LOCK();
-   error = sysctl_int(oldp, oldlenp, newp, newlen, _wccp);
-   NET_UNLOCK();
-   return (error);
-   default:
-   return (ENOPROTOOPT);
-   }
-   /* NOTREACHED */
+   NET_LOCK();
+   error = sysctl_bounded_arr(gre_vars, nitems(gre_vars), name,
+   namelen, oldp, oldlenp, newp, newlen);
+   NET_UNLOCK();
+   return error;
 }
 
 static inline int
-- 
2.29.2