[PATCH net-next] neighbor: fix proxy_delay usage when it is zero

[PATCH net-next] neighbor: fix proxy_delay usage when it is zero

[Brian Haley]

When set to zero, the neighbor sysctl proxy_delay value
does not cause an immediate reply for ARP/ND requests
as expected, it instead causes a random delay between
[0, U32_MAX]. Looking at this comment from
__get_random_u32_below() explains the reason:

/*
 * This function is technically undefined for ceil == 0, and in fact
 * for the non-underscored constant version in the header, we build bug
 * on that. But for the non-constant case, it's convenient to have that
 * evaluate to being a straight call to get_random_u32(), so that
 * get_random_u32_inclusive() can work over its whole range without
 * undefined behavior.
 */

Added helper function that does not call get_random_u32_below()
if proxy_delay is zero and just uses the current value of
jiffies instead, causing pneigh_enqueue() to respond
immediately.

Also added definition of proxy_delay to ip-sysctl.txt since
it was missing.

Signed-off-by: Brian Haley <haleyb.dev@gmail.com>
---
 Documentation/networking/ip-sysctl.rst |  6 ++++++
 net/core/neighbour.c                   | 15 +++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 7fbd060d6047..34183fb38b20 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -1589,6 +1589,12 @@ proxy_arp_pvlan - BOOLEAN
 	  Hewlett-Packard call it Source-Port filtering or port-isolation.
 	  Ericsson call it MAC-Forced Forwarding (RFC Draft).
 
+proxy_delay - INTEGER
+	Delay proxy response.
+
+	The maximum number of jiffies to delay a response to a neighbor
+	solicitation when proxy_arp or proxy_ndp is enabled. Defaults to 80.
+
 shared_media - BOOLEAN
 	Send(router) or accept(host) RFC1620 shared media redirects.
 	Overrides secure_redirects.
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index f00a79fc301b..8bd8aaae6d5e 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1662,11 +1662,22 @@ static void neigh_proxy_process(struct timer_list *t)
 	spin_unlock(&tbl->proxy_queue.lock);
 }
 
+static __inline__ unsigned long neigh_proxy_delay(struct neigh_parms *p)
+{
+	/*
+	 * If proxy_delay is zero, do not call get_random_u32_below()
+	 * as it is undefined behavior.
+	 */
+	unsigned long proxy_delay = NEIGH_VAR(p, PROXY_DELAY);
+	return proxy_delay ?
+	       jiffies + get_random_u32_below(NEIGH_VAR(p, PROXY_DELAY)) :
+	       jiffies;
+}
+
 void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
 		    struct sk_buff *skb)
 {
-	unsigned long sched_next = jiffies +
-			get_random_u32_below(NEIGH_VAR(p, PROXY_DELAY));
+	unsigned long sched_next = neigh_proxy_delay(p);
 
 	if (p->qlen > NEIGH_VAR(p, PROXY_QLEN)) {
 		kfree_skb(skb);
-- 
2.34.1

Re: [PATCH net-next] neighbor: fix proxy_delay usage when it is zero

[Jakub Kicinski]

On Mon, 23 Jan 2023 13:58:29 -0500 Brian Haley wrote:
> When set to zero, the neighbor sysctl proxy_delay value
> does not cause an immediate reply for ARP/ND requests
> as expected, it instead causes a random delay between
> [0, U32_MAX]. Looking at this comment from
> __get_random_u32_below() explains the reason:
> 
> /*
>  * This function is technically undefined for ceil == 0, and in fact
>  * for the non-underscored constant version in the header, we build bug
>  * on that. But for the non-constant case, it's convenient to have that
>  * evaluate to being a straight call to get_random_u32(), so that
>  * get_random_u32_inclusive() can work over its whole range without
>  * undefined behavior.
>  */
> 
> Added helper function that does not call get_random_u32_below()
> if proxy_delay is zero and just uses the current value of
> jiffies instead, causing pneigh_enqueue() to respond
> immediately.
> 
> Also added definition of proxy_delay to ip-sysctl.txt since
> it was missing.

Sounds like this never worked, until commit a533b70a657c ("net:
neighbor: fix a crash caused by mod zero") it crashed, now it
does something silly. Can we instead reject 0 as invalid input
during configuration?

> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 7fbd060d6047..34183fb38b20 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -1589,6 +1589,12 @@ proxy_arp_pvlan - BOOLEAN
>  	  Hewlett-Packard call it Source-Port filtering or port-isolation.
>  	  Ericsson call it MAC-Forced Forwarding (RFC Draft).
>  
> +proxy_delay - INTEGER
> +	Delay proxy response.
> +
> +	The maximum number of jiffies to delay a response to a neighbor
> +	solicitation when proxy_arp or proxy_ndp is enabled. Defaults to 80.

Is there a better way of expressing the fact that we always
choose a value lower than proxy_delay ? Maximum sounds a bit
like we'd do:

	when = jiffies + random() % (proxy_delay + 1);

>  shared_media - BOOLEAN
>  	Send(router) or accept(host) RFC1620 shared media redirects.
>  	Overrides secure_redirects.
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index f00a79fc301b..8bd8aaae6d5e 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1662,11 +1662,22 @@ static void neigh_proxy_process(struct timer_list *t)
>  	spin_unlock(&tbl->proxy_queue.lock);
>  }
>  
> +static __inline__ unsigned long neigh_proxy_delay(struct neigh_parms *p)

Drop the inline please, it's pointless for a tiny static function

> +{
> +	/*

did you run checkpatch?

> +	 * If proxy_delay is zero, do not call get_random_u32_below()
> +	 * as it is undefined behavior.
> +	 */
> +	unsigned long proxy_delay = NEIGH_VAR(p, PROXY_DELAY);

empty line here

> +	return proxy_delay ?
> +	       jiffies + get_random_u32_below(NEIGH_VAR(p, PROXY_DELAY)) :

also - since you have proxy_delay in a local variable why not use it

> +	       jiffies;
> +}
> +
>  void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
>  		    struct sk_buff *skb)
>  {
> -	unsigned long sched_next = jiffies +
> -			get_random_u32_below(NEIGH_VAR(p, PROXY_DELAY));
> +	unsigned long sched_next = neigh_proxy_delay(p);
>  
>  	if (p->qlen > NEIGH_VAR(p, PROXY_QLEN)) {
>  		kfree_skb(skb);

Re: [PATCH net-next] neighbor: fix proxy_delay usage when it is zero

[Brian Haley]

Jakub - thanks for the review.

On 1/24/23 11:30 PM, Jakub Kicinski wrote:
> On Mon, 23 Jan 2023 13:58:29 -0500 Brian Haley wrote:
>> When set to zero, the neighbor sysctl proxy_delay value
>> does not cause an immediate reply for ARP/ND requests
>> as expected, it instead causes a random delay between
>> [0, U32_MAX]. Looking at this comment from
>> __get_random_u32_below() explains the reason:
>>
>> /*
>>   * This function is technically undefined for ceil == 0, and in fact
>>   * for the non-underscored constant version in the header, we build bug
>>   * on that. But for the non-constant case, it's convenient to have that
>>   * evaluate to being a straight call to get_random_u32(), so that
>>   * get_random_u32_inclusive() can work over its whole range without
>>   * undefined behavior.
>>   */
>>
>> Added helper function that does not call get_random_u32_below()
>> if proxy_delay is zero and just uses the current value of
>> jiffies instead, causing pneigh_enqueue() to respond
>> immediately.
>>
>> Also added definition of proxy_delay to ip-sysctl.txt since
>> it was missing.
> 
> Sounds like this never worked, until commit a533b70a657c ("net:
> neighbor: fix a crash caused by mod zero") it crashed, now it
> does something silly. Can we instead reject 0 as invalid input
> during configuration?

To me, proxy_delay==0 implies respond immediately, so I think zero is 
valid input.

>> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
>> index 7fbd060d6047..34183fb38b20 100644
>> --- a/Documentation/networking/ip-sysctl.rst
>> +++ b/Documentation/networking/ip-sysctl.rst
>> @@ -1589,6 +1589,12 @@ proxy_arp_pvlan - BOOLEAN
>>   	  Hewlett-Packard call it Source-Port filtering or port-isolation.
>>   	  Ericsson call it MAC-Forced Forwarding (RFC Draft).
>>   
>> +proxy_delay - INTEGER
>> +	Delay proxy response.
>> +
>> +	The maximum number of jiffies to delay a response to a neighbor
>> +	solicitation when proxy_arp or proxy_ndp is enabled. Defaults to 80.
> 
> Is there a better way of expressing the fact that we always
> choose a value lower than proxy_delay ? Maximum sounds a bit
> like we'd do:
> 
> 	when = jiffies + random() % (proxy_delay + 1);

I think technically it's in the range of [0, proxy_delay] but I'll 
change the text to make that more obvious.

>>   shared_media - BOOLEAN
>>   	Send(router) or accept(host) RFC1620 shared media redirects.
>>   	Overrides secure_redirects.
>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>> index f00a79fc301b..8bd8aaae6d5e 100644
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
>> @@ -1662,11 +1662,22 @@ static void neigh_proxy_process(struct timer_list *t)
>>   	spin_unlock(&tbl->proxy_queue.lock);
>>   }
>>   
>> +static __inline__ unsigned long neigh_proxy_delay(struct neigh_parms *p)
> 
> Drop the inline please, it's pointless for a tiny static function

JayV mentioned the same in private, guess I owe him a beer.

>> +{
>> +	/*
> 
> did you run checkpatch?

No, my kernel-fu is a little rusty, will do on v2.

> 
>> +	 * If proxy_delay is zero, do not call get_random_u32_below()
>> +	 * as it is undefined behavior.
>> +	 */
>> +	unsigned long proxy_delay = NEIGH_VAR(p, PROXY_DELAY);
> 
> empty line here
> 
>> +	return proxy_delay ?
>> +	       jiffies + get_random_u32_below(NEIGH_VAR(p, PROXY_DELAY)) :
> 
> also - since you have proxy_delay in a local variable why not use it

Ack.

Thanks,

-Brian