[PATCH net-next] arp: flush arp cache on IFF_NOARP change

[PATCH net-next] arp: flush arp cache on IFF_NOARP change

[Timo Teräs]

IFF_NOARP affects what kind of neighbor entries are created
(nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
cache to refresh all entries.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
Cc: Patrick McHardy <kaber@trash.net>
---
Original patch sent some years ago and the review is at:
 http://patchwork.ozlabs.org/patch/46806/

This is updated per Patrick's suggestion to use priv_flags as the
place to notify that NOARP flag changed.

 include/uapi/linux/if.h | 1 +
 net/core/dev.c          | 5 +++++
 net/ipv4/arp.c          | 4 ++++
 3 files changed, 10 insertions(+)

diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index 1ec407b..ace4aec 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -83,6 +83,7 @@
 #define IFF_SUPP_NOFCS	0x80000		/* device supports sending custom FCS */
 #define IFF_LIVE_ADDR_CHANGE 0x100000	/* device supports hardware address
 					 * change when it's running */
+#define IFF_NOARP_CHANGED 0x200000	/* IFF_NOARP changed state recently */
 
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
diff --git a/net/core/dev.c b/net/core/dev.c
index 18e9730..7135fe1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4691,6 +4691,11 @@ void __dev_notify_flags(struct net_device *dev, unsigned int old_flags)
 {
 	unsigned int changes = dev->flags ^ old_flags;
 
+	if (changes & IFF_NOARP)
+		dev->priv_flags |= IFF_NOARP_CHANGED;
+	else
+		dev->priv_flags &= ~IFF_NOARP_CHANGED;
+
 	if (changes & IFF_UP) {
 		if (dev->flags & IFF_UP)
 			call_netdevice_notifiers(NETDEV_UP, dev);
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 247ec19..375b2f2 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -1241,6 +1241,10 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
 		neigh_changeaddr(&arp_tbl, dev);
 		rt_cache_flush(dev_net(dev));
 		break;
+	case NETDEV_CHANGE:
+		if (dev->priv_flags & IFF_NOARP_CHANGED)
+			neigh_changeaddr(&arp_tbl, dev);
+		break;
 	default:
 		break;
 	}
-- 
1.8.2.3

Re: [PATCH net-next] arp: flush arp cache on IFF_NOARP change

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Mon, 20 May 2013 16:29:01 +0300

> IFF_NOARP affects what kind of neighbor entries are created
> (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> cache to refresh all entries.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> Cc: Patrick McHardy <kaber@trash.net>

This patch makes no sense at all.

The state bit in ->priv_flags is a boolean stating whether the
notified should do something or not.

But you're setting it to match what IFF_NOARP is.

You should set it any time IFF_NOARP _changes_, and then clear
the bit when the notifier clears the neighbour entries.

[PATCH v2 net-next] arp: flush arp cache on IFF_NOARP change

[Timo Teräs]

IFF_NOARP affects what kind of neighbor entries are created
(nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
cache to refresh all entries.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
> This patch makes no sense at all.
> 
> The state bit in ->priv_flags is a boolean stating whether the
> notified should do something or not.
> 
> But you're setting it to match what IFF_NOARP is.
>
> You should set it any time IFF_NOARP _changes_, and then clear
> the bit when the notifier clears the neighbour entries.

IFF_NOARP_CHANGED is set according to "changes = dev->flags ^ old_flags;"
which reflect the change. But I agree that the clearing out bit was
misplaced. This is especially true as it seems NETDEV_CHANGE can be
notified from another place too.

I've updated the if.h comment to state that the bit is valid only during
NETDEV_CHANGE notifier. And __dev_notify_flags is updated to always clear
the bit after notifiers are done.

 include/uapi/linux/if.h | 2 ++
 net/core/dev.c          | 6 +++++-
 net/ipv4/arp.c          | 4 ++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index 1ec407b..1be8b35 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -83,6 +83,8 @@
 #define IFF_SUPP_NOFCS	0x80000		/* device supports sending custom FCS */
 #define IFF_LIVE_ADDR_CHANGE 0x100000	/* device supports hardware address
 					 * change when it's running */
+#define IFF_NOARP_CHANGED 0x200000	/* Set during NETDEV_CHANGE notifier
+					 * if IFF_NOARP has changed */
 
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
diff --git a/net/core/dev.c b/net/core/dev.c
index 18e9730..ce30761 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4699,8 +4699,12 @@ void __dev_notify_flags(struct net_device *dev, unsigned int old_flags)
 	}
 
 	if (dev->flags & IFF_UP &&
-	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE)))
+	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE))) {
+		if (changes & IFF_NOARP)
+			dev->priv_flags |= IFF_NOARP_CHANGED;
 		call_netdevice_notifiers(NETDEV_CHANGE, dev);
+		dev->priv_flags &= ~IFF_NOARP_CHANGED;
+	}
 }
 
 /**
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 247ec19..375b2f2 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -1241,6 +1241,10 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
 		neigh_changeaddr(&arp_tbl, dev);
 		rt_cache_flush(dev_net(dev));
 		break;
+	case NETDEV_CHANGE:
+		if (dev->priv_flags & IFF_NOARP_CHANGED)
+			neigh_changeaddr(&arp_tbl, dev);
+		break;
 	default:
 		break;
 	}
-- 
1.8.2.3

Re: [PATCH v2 net-next] arp: flush arp cache on IFF_NOARP change

[Ben Hutchings]

On Tue, 2013-05-21 at 13:23 +0300, Timo Teräs wrote:
> IFF_NOARP affects what kind of neighbor entries are created
> (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> cache to refresh all entries.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
> > This patch makes no sense at all.
> > 
> > The state bit in ->priv_flags is a boolean stating whether the
> > notified should do something or not.
> > 
> > But you're setting it to match what IFF_NOARP is.
> >
> > You should set it any time IFF_NOARP _changes_, and then clear
> > the bit when the notifier clears the neighbour entries.
> 
> IFF_NOARP_CHANGED is set according to "changes = dev->flags ^ old_flags;"
> which reflect the change. But I agree that the clearing out bit was
> misplaced. This is especially true as it seems NETDEV_CHANGE can be
> notified from another place too.
[...]

None of the other persistent flags have a transient flag for changes;
why should IFF_NOARP?  Why not cache the IFF_NOARP state in the
in_device and then compare and update that in arp_netdev_event().

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH v2 net-next] arp: flush arp cache on IFF_NOARP change

[Timo Teras]

On Tue, 21 May 2013 17:25:19 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Tue, 2013-05-21 at 13:23 +0300, Timo Teräs wrote:
> > IFF_NOARP affects what kind of neighbor entries are created
> > (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> > cache to refresh all entries.
> > 
> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> > ---
> > > This patch makes no sense at all.
> > > 
> > > The state bit in ->priv_flags is a boolean stating whether the
> > > notified should do something or not.
> > > 
> > > But you're setting it to match what IFF_NOARP is.
> > >
> > > You should set it any time IFF_NOARP _changes_, and then clear
> > > the bit when the notifier clears the neighbour entries.
> > 
> > IFF_NOARP_CHANGED is set according to "changes = dev->flags ^
> > old_flags;" which reflect the change. But I agree that the clearing
> > out bit was misplaced. This is especially true as it seems
> > NETDEV_CHANGE can be notified from another place too.
> [...]
> 
> None of the other persistent flags have a transient flag for changes;
> why should IFF_NOARP?  Why not cache the IFF_NOARP state in the
> in_device and then compare and update that in arp_netdev_event().

This was the earlier suggestion I got, so I followed that. NOARP flag
is also used in IPv6 side (quick look would indicate that ndisc code
needs similar fix), so it would be useful to have this generally
available.

Would it be worthwhile to consider adding "flags_changed" to struct
net_device or some other mechanism add this info about all flags?

Re: [PATCH v2 net-next] arp: flush arp cache on IFF_NOARP change

[Ben Hutchings]

On Tue, 2013-05-21 at 21:29 +0300, Timo Teras wrote:
> On Tue, 21 May 2013 17:25:19 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Tue, 2013-05-21 at 13:23 +0300, Timo Teräs wrote:
> > > IFF_NOARP affects what kind of neighbor entries are created
> > > (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> > > cache to refresh all entries.
> > > 
> > > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> > > ---
> > > > This patch makes no sense at all.
> > > > 
> > > > The state bit in ->priv_flags is a boolean stating whether the
> > > > notified should do something or not.
> > > > 
> > > > But you're setting it to match what IFF_NOARP is.
> > > >
> > > > You should set it any time IFF_NOARP _changes_, and then clear
> > > > the bit when the notifier clears the neighbour entries.
> > > 
> > > IFF_NOARP_CHANGED is set according to "changes = dev->flags ^
> > > old_flags;" which reflect the change. But I agree that the clearing
> > > out bit was misplaced. This is especially true as it seems
> > > NETDEV_CHANGE can be notified from another place too.
> > [...]
> > 
> > None of the other persistent flags have a transient flag for changes;
> > why should IFF_NOARP?  Why not cache the IFF_NOARP state in the
> > in_device and then compare and update that in arp_netdev_event().
> 
> This was the earlier suggestion I got, so I followed that. NOARP flag
> is also used in IPv6 side (quick look would indicate that ndisc code
> needs similar fix), so it would be useful to have this generally
> available.

Sorry, I missed that IFF_NOARP is not really just about ARP.

> Would it be worthwhile to consider adding "flags_changed" to struct
> net_device or some other mechanism add this info about all flags?

It's inelegant to put transient data associated with an event in a
persistent data structure.  On the other hand, having every user cache
the old state is pretty awful as well.

Really, netdev notifiers should be changed to accept a structure that
encapsulates the changes rather than just a pointer to the net_device.
But making such a change would be an enormous pain and error-prone
because notifier functions aren't type-safe.

As an interim solution, I think either the general priv_flags_changed or
old_priv_flags would be preferable to defining extra transient flags.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH v2 net-next] arp: flush arp cache on IFF_NOARP change

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Tue, 21 May 2013 13:23:44 +0300

> IFF_NOARP affects what kind of neighbor entries are created
> (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> cache to refresh all entries.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>

I agree with Ben that we should have a "orig_priv_flags" or
something like that to implement this, rather than adding
transient state flags to ->priv_flags.

Re: [PATCH v2 net-next] arp: flush arp cache on IFF_NOARP change

[Timo Teras]

On Thu, 23 May 2013 00:01:36 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Timo Teräs <timo.teras@iki.fi>
> Date: Tue, 21 May 2013 13:23:44 +0300
> 
> > IFF_NOARP affects what kind of neighbor entries are created
> > (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> > cache to refresh all entries.
> > 
> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> 
> I agree with Ben that we should have a "orig_priv_flags" or
> something like that to implement this, rather than adding
> transient state flags to ->priv_flags.

Sure. I'll split this to two patches then and do this.

I would prefer "flags_changed" as then the notifiers can easily test on
the change. Usually the care only about the new state, or if it
changed. Otherwise the change calculation is needed on all notify
callbacks.

Will post this briefly, unless you feel that "orig_flags" is still
preferred.