| Message ID | 1340892639-1111-2-git-send-email-jpirko@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote: > Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable > netif_running() check in eth_mac_addr() > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> > --- > include/linux/if.h | 2 ++ > net/ethernet/eth.c | 2 +- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/if.h b/include/linux/if.h > index f995c66..fd9ee7c 100644 > --- a/include/linux/if.h > +++ b/include/linux/if.h > @@ -81,6 +81,8 @@ > #define IFF_UNICAST_FLT 0x20000 /* Supports unicast filtering */ > #define IFF_TEAM_PORT 0x40000 /* device used as team port */ > #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */ > +#define IFF_LIFE_ADDR_CHANGE 0x100000 /* device supports hardware address > + * change when it's running */ > > > #define IF_GET_IFACE 0x0001 /* for querying only */ > diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c > index 36e5880..8f8ded4 100644 > --- a/net/ethernet/eth.c > +++ b/net/ethernet/eth.c > @@ -283,7 +283,7 @@ int eth_mac_addr(struct net_device *dev, void *p) > { > struct sockaddr *addr = p; > > - if (netif_running(dev)) > + if (!(dev->priv_flags & IFF_LIFE_ADDR_CHANGE) && netif_running(dev)) > return -EBUSY; > if (!is_valid_ether_addr(addr->sa_data)) > return -EADDRNOTAVAIL; Since the memcpy() is not atomic, there is a small window where a reader could get a half-changed mac address. I guess its a detail. -- 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
On Thu, Jun 28, 2012 at 05:24:52PM +0200, Eric Dumazet wrote: > On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote: > > Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable > > netif_running() check in eth_mac_addr() > > > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> > > --- > > include/linux/if.h | 2 ++ > > net/ethernet/eth.c | 2 +- > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/if.h b/include/linux/if.h > > index f995c66..fd9ee7c 100644 > > --- a/include/linux/if.h > > +++ b/include/linux/if.h > > @@ -81,6 +81,8 @@ > > #define IFF_UNICAST_FLT 0x20000 /* Supports unicast filtering */ > > #define IFF_TEAM_PORT 0x40000 /* device used as team port */ > > #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */ > > +#define IFF_LIFE_ADDR_CHANGE 0x100000 /* device supports hardware address > > + * change when it's running */ > > > > > > #define IF_GET_IFACE 0x0001 /* for querying only */ > > diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c > > index 36e5880..8f8ded4 100644 > > --- a/net/ethernet/eth.c > > +++ b/net/ethernet/eth.c > > @@ -283,7 +283,7 @@ int eth_mac_addr(struct net_device *dev, void *p) > > { > > struct sockaddr *addr = p; > > > > - if (netif_running(dev)) > > + if (!(dev->priv_flags & IFF_LIFE_ADDR_CHANGE) && netif_running(dev)) > > return -EBUSY; > > if (!is_valid_ether_addr(addr->sa_data)) > > return -EADDRNOTAVAIL; > > Since the memcpy() is not atomic, there is a small window where a reader > could get a half-changed mac address. I guess its a detail. > At least for virtio nothing changes - we had this bug forever. How'd you fix this?
On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote: > Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable > netif_running() check in eth_mac_addr() > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> > --- > include/linux/if.h | 2 ++ > net/ethernet/eth.c | 2 +- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/if.h b/include/linux/if.h > index f995c66..fd9ee7c 100644 > --- a/include/linux/if.h > +++ b/include/linux/if.h > @@ -81,6 +81,8 @@ > #define IFF_UNICAST_FLT 0x20000 /* Supports unicast filtering */ > #define IFF_TEAM_PORT 0x40000 /* device used as team port */ > #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */ > +#define IFF_LIFE_ADDR_CHANGE 0x100000 /* device supports hardware address > + * change when it's running */ [...] Any device that has IFF_UNICAST_FLT can update the unicast MAC filter while it's running; doesn't that go hand-in-hand with being able to handle changes to the primary MAC address? Is the new flag really necessary at all? Ben.
Fri, Jun 29, 2012 at 12:17:34AM CEST, bhutchings@solarflare.com wrote: >On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote: >> Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable >> netif_running() check in eth_mac_addr() >> >> Signed-off-by: Jiri Pirko <jpirko@redhat.com> >> --- >> include/linux/if.h | 2 ++ >> net/ethernet/eth.c | 2 +- >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/if.h b/include/linux/if.h >> index f995c66..fd9ee7c 100644 >> --- a/include/linux/if.h >> +++ b/include/linux/if.h >> @@ -81,6 +81,8 @@ >> #define IFF_UNICAST_FLT 0x20000 /* Supports unicast filtering */ >> #define IFF_TEAM_PORT 0x40000 /* device used as team port */ >> #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */ >> +#define IFF_LIFE_ADDR_CHANGE 0x100000 /* device supports hardware address >> + * change when it's running */ >[...] > >Any device that has IFF_UNICAST_FLT can update the unicast MAC filter >while it's running; doesn't that go hand-in-hand with being able to >handle changes to the primary MAC address? Is the new flag really >necessary at all? Hmm, this makes sense. But, can you guarantee that all devices behave like this? Also, there are many devices that does not support unicast filtering and yet they support updating mac adress while running. Jirka > >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. > -- 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 --git a/include/linux/if.h b/include/linux/if.h index f995c66..fd9ee7c 100644 --- a/include/linux/if.h +++ b/include/linux/if.h @@ -81,6 +81,8 @@ #define IFF_UNICAST_FLT 0x20000 /* Supports unicast filtering */ #define IFF_TEAM_PORT 0x40000 /* device used as team port */ #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */ +#define IFF_LIFE_ADDR_CHANGE 0x100000 /* device supports hardware address + * change when it's running */ #define IF_GET_IFACE 0x0001 /* for querying only */ diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 36e5880..8f8ded4 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -283,7 +283,7 @@ int eth_mac_addr(struct net_device *dev, void *p) { struct sockaddr *addr = p; - if (netif_running(dev)) + if (!(dev->priv_flags & IFF_LIFE_ADDR_CHANGE) && netif_running(dev)) return -EBUSY; if (!is_valid_ether_addr(addr->sa_data)) return -EADDRNOTAVAIL;
Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable netif_running() check in eth_mac_addr() Signed-off-by: Jiri Pirko <jpirko@redhat.com> --- include/linux/if.h | 2 ++ net/ethernet/eth.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)