diff mbox

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

Message ID 1369131824-6318-1-git-send-email-timo.teras@iki.fi
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Timo Teras May 21, 2013, 10:23 a.m. UTC 
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(-)

Comments

Ben Hutchings May 21, 2013, 4:25 p.m. UTC | #1 
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.
Timo Teras May 21, 2013, 6:29 p.m. UTC | #2 
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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings May 21, 2013, 6:54 p.m. UTC | #3 
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.
David Miller May 23, 2013, 7:01 a.m. UTC | #4 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Timo Teras May 23, 2013, 7:49 a.m. UTC | #5 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 	}