[patch net-next] neigh: use NEIGH_VAR_INIT in ndo_neigh_setup functions.

[patch net-next] neigh: use NEIGH_VAR_INIT in ndo_neigh_setup functions.

[Jiri Pirko]

When ndo_neigh_setup is called, the bitfield used by NEIGH_VAR_SET is
not initialized yet. This might cause confusion for the people who use
NEIGH_VAR_SET in ndo_neigh_setup. So rather introduce NEIGH_VAR_INIT for
usage in ndo_neigh_setup.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/net/neighbour.h | 5 +++++
 net/802/hippi.c         | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index 4c09bd2..7277caf 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -92,6 +92,11 @@ static inline void neigh_var_set(struct neigh_parms *p, int index, int val)
 }
 
 #define NEIGH_VAR(p, attr) ((p)->data[NEIGH_VAR_ ## attr])
+
+/* In ndo_neigh_setup, NEIGH_VAR_INIT should be used.
+ * In other cases, NEIGH_VAR_SET should be used.
+ */
+#define NEIGH_VAR_INIT(p, attr, val) (NEIGH_VAR(p, attr) = val)
 #define NEIGH_VAR_SET(p, attr, val) neigh_var_set(p, NEIGH_VAR_ ## attr, val)
 
 static inline void neigh_parms_data_state_setall(struct neigh_parms *p)
diff --git a/net/802/hippi.c b/net/802/hippi.c
index a97a3bd..5ff2a71 100644
--- a/net/802/hippi.c
+++ b/net/802/hippi.c
@@ -172,14 +172,14 @@ EXPORT_SYMBOL(hippi_mac_addr);
 int hippi_neigh_setup_dev(struct net_device *dev, struct neigh_parms *p)
 {
 	/* Never send broadcast/multicast ARP messages */
-	NEIGH_VAR_SET(p, MCAST_PROBES, 0);
+	NEIGH_VAR_INIT(p, MCAST_PROBES, 0);
 
 	/* In IPv6 unicast probes are valid even on NBMA,
 	* because they are encapsulated in normal IPv6 protocol.
 	* Should be a generic flag.
 	*/
 	if (p->tbl->family != AF_INET6)
-		NEIGH_VAR_SET(p, UCAST_PROBES, 0);
+		NEIGH_VAR_INIT(p, UCAST_PROBES, 0);
 	return 0;
 }
 EXPORT_SYMBOL(hippi_neigh_setup_dev);
-- 
1.8.3.1

Re: [patch net-next] neigh: use NEIGH_VAR_INIT in ndo_neigh_setup functions.

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu,  9 Jan 2014 14:13:47 +0100

> When ndo_neigh_setup is called, the bitfield used by NEIGH_VAR_SET is
> not initialized yet. This might cause confusion for the people who use
> NEIGH_VAR_SET in ndo_neigh_setup. So rather introduce NEIGH_VAR_INIT for
> usage in ndo_neigh_setup.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Wouldn't it be better to move the neigh_parms_data_state_cleanall() call
before we invoke ->ndo_neigh_setup()?  It seems that this code intended
to work that way, no?

Re: [patch net-next] neigh: use NEIGH_VAR_INIT in ndo_neigh_setup functions.

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu,  9 Jan 2014 14:13:47 +0100

> When ndo_neigh_setup is called, the bitfield used by NEIGH_VAR_SET is
> not initialized yet. This might cause confusion for the people who use
> NEIGH_VAR_SET in ndo_neigh_setup. So rather introduce NEIGH_VAR_INIT for
> usage in ndo_neigh_setup.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Jiri, please respond to my feedback, this patch has been rotting in
patchwork for 6 days.

Re: [patch net-next] neigh: use NEIGH_VAR_INIT in ndo_neigh_setup functions.

[Jiri Pirko]

Mon, Jan 13, 2014 at 08:35:38PM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu,  9 Jan 2014 14:13:47 +0100
>
>> When ndo_neigh_setup is called, the bitfield used by NEIGH_VAR_SET is
>> not initialized yet. This might cause confusion for the people who use
>> NEIGH_VAR_SET in ndo_neigh_setup. So rather introduce NEIGH_VAR_INIT for
>> usage in ndo_neigh_setup.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
>Wouldn't it be better to move the neigh_parms_data_state_cleanall() call
>before we invoke ->ndo_neigh_setup()?  It seems that this code intended
>to work that way, no?

Even moving neigh_parms_data_state_cleanall before ndo_neigh_setup
would not solve this. In ndo_neigh_setup the original default value is
changed. If it is changed by NEIGH_VAR_SET, it touches data_state bit
and it looks as if it was changed by user. That is not desired because
default value change would be ignored for this device. Therefore there
is need for NEIGH_VAR_INIT which does not touch data_state bit.

Re: [patch net-next] neigh: use NEIGH_VAR_INIT in ndo_neigh_setup functions.

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 16 Jan 2014 08:15:19 +0100

> Mon, Jan 13, 2014 at 08:35:38PM CET, davem@davemloft.net wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Date: Thu,  9 Jan 2014 14:13:47 +0100
>>
>>> When ndo_neigh_setup is called, the bitfield used by NEIGH_VAR_SET is
>>> not initialized yet. This might cause confusion for the people who use
>>> NEIGH_VAR_SET in ndo_neigh_setup. So rather introduce NEIGH_VAR_INIT for
>>> usage in ndo_neigh_setup.
>>> 
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>
>>Wouldn't it be better to move the neigh_parms_data_state_cleanall() call
>>before we invoke ->ndo_neigh_setup()?  It seems that this code intended
>>to work that way, no?
> 
> Even moving neigh_parms_data_state_cleanall before ndo_neigh_setup
> would not solve this. In ndo_neigh_setup the original default value is
> changed. If it is changed by NEIGH_VAR_SET, it touches data_state bit
> and it looks as if it was changed by user. That is not desired because
> default value change would be ignored for this device. Therefore there
> is need for NEIGH_VAR_INIT which does not touch data_state bit.

That makes sense, applied, thanks Jiri.