* [RFC PATCH] core: Add ioctls to control device unicast hw addresses @ 2013-02-26 16:59 Vlad Yasevich 2013-02-26 17:43 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Vlad Yasevich @ 2013-02-26 16:59 UTC (permalink / raw) To: netdev There is currently a way to manage the multicast HW address list through SIOCADDMULTI/SIOCDELMULTI, but there is no way to consistently manage the unicast list. Some drivers provide FDB based interface, but that depends on driver configuration in almost every case. This patch provides 2 new ioctls that allow on to add/delete unicast HW address on a device. Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- include/uapi/linux/sockios.h | 4 ++++ net/core/dev_ioctl.c | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 0 deletions(-) diff --git a/include/uapi/linux/sockios.h b/include/uapi/linux/sockios.h index 7997a50..442360e 100644 --- a/include/uapi/linux/sockios.h +++ b/include/uapi/linux/sockios.h @@ -127,6 +127,10 @@ /* hardware time stamping: parameters in linux/net_tstamp.h */ #define SIOCSHWTSTAMP 0x89b0 +/* Device address management */ +#define SIOCADDHWADDR 0x89c0 /* Unicast address list */ +#define SIOCDELHWADDR 0x8932 + /* Device private ioctl calls */ /* diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 6cc0481..d84f6f9 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -314,6 +314,26 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd) ifr->ifr_newname[IFNAMSIZ-1] = '\0'; return dev_change_name(dev, ifr->ifr_newname); + case SIOCADDHWADDR: + if (!ops->ndo_set_rx_mode || + ifr->ifr_hwaddr.sa_family != AF_UNSPEC) + return -EINVAL; + if (!netif_device_present(dev)) + return -ENODEV; + if (!is_unicast_ether_addr(addr)) + return -EINVAL; + return dev_uc_add_excl(dev, ifr->ifr_hwaddr.sa_data); + + case SIOCDELHWADDR: + if (!ops->ndo_set_rx_mode || + ifr->ifr_hwaddr.sa_family != AF_UNSPEC) + return -EINVAL; + if (!netif_device_present(dev)) + return -ENODEV; + if (!is_unicast_ether_addr(addr)) + return -EINVAL; + return dev_uc_del(dev, ifr->ifr_hwaddr.sa_data); + case SIOCSHWTSTAMP: err = net_hwtstamp_validate(ifr); if (err) @@ -532,6 +552,8 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg) case SIOCBRADDIF: case SIOCBRDELIF: case SIOCSHWTSTAMP: + case SIOCADDHWADDR: + case SIOCDELHWADDR: if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) return -EPERM; /* fall through */ -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 16:59 [RFC PATCH] core: Add ioctls to control device unicast hw addresses Vlad Yasevich @ 2013-02-26 17:43 ` David Miller 2013-02-26 19:44 ` Vlad Yasevich 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2013-02-26 17:43 UTC (permalink / raw) To: vyasevic; +Cc: netdev No new ioctls please, extend the netlink interfaces as necessary instead. Thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 17:43 ` David Miller @ 2013-02-26 19:44 ` Vlad Yasevich 2013-02-26 20:03 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Vlad Yasevich @ 2013-02-26 19:44 UTC (permalink / raw) To: davem; +Cc: netdev David Miller wrote: > No new ioctls please, extend the netlink interfaces as necessary > instead. > > Thanks. So, something like below patch (compiled only) would be better in your opinion? The one concern I have is that there is no way to probe for the availabilty of this functionality, since netlink will just ignore unknown types and appear to succeed. With an ioctl it is clear if the support is present. -- >8 -- [RFC PATCH] rtnetlink: Add support for adding/removing additional hw addresses. Add an ability to add and remove HW addresses to the device unicast and multicast address lists. Right now, we only have an ioctl() to manage the multicast addresses and there is no way the manage the unicast list. Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- include/uapi/linux/if_link.h | 3 + include/uapi/linux/rtnetlink.h | 1 + net/core/rtnetlink.c | 84 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 87 insertions(+), 1 deletions(-) diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index c4edfe1..7933a1c 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -143,6 +143,9 @@ enum { IFLA_NUM_TX_QUEUES, IFLA_NUM_RX_QUEUES, IFLA_CARRIER, + IFLA_HW_ADDR_ADD, + IFLA_HW_ADDR_DEL, + IFLA_HW_ADDR_LIST, __IFLA_MAX }; diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 7a2144e..4d6f1c9 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -631,6 +631,7 @@ struct tcamsg { /* New extended info filters for IFLA_EXT_MASK */ #define RTEXT_FILTER_VF (1 << 0) #define RTEXT_FILTER_BRVLAN (1 << 1) +#define RTEXT_FILTER_MACLIST (1 << 2) /* End of information exported to user level */ diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d8aa20f..4df2ff1 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -763,6 +763,20 @@ static size_t rtnl_port_size(const struct net_device *dev) return port_self_size; } +static size_t rtnl_maclist_size(const struct net_device *dev, + u32 ext_filter_mask) +{ + size_t size = 0; + + if (ext_filter_mask & RTEXT_FILTER_MACLIST) { + size = nla_total_size(sizeof(struct nlattr)) + + nla_total_size(ETH_ALEN) * + (netdev_uc_count(dev) + netdev_mc_count(dev)); + } + + return size; +} + static noinline size_t if_nlmsg_size(const struct net_device *dev, u32 ext_filter_mask) { @@ -790,6 +804,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev, & RTEXT_FILTER_VF ? 4 : 0) /* IFLA_NUM_VF */ + rtnl_vfinfo_size(dev, ext_filter_mask) /* IFLA_VFINFO_LIST */ + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */ + + rtnl_maclist_size(dev, ext_filter_mask) /* IFLA_MAC_LIST */ + rtnl_link_get_size(dev) /* IFLA_LINKINFO */ + rtnl_link_get_af_size(dev); /* IFLA_AF_SPEC */ } @@ -870,6 +885,39 @@ static int rtnl_port_fill(struct sk_buff *skb, struct net_device *dev) return 0; } +static int rtnl_hwaddr_fill(struct sk_buff *skb, struct net_device *dev, + u32 ext_filter_mask) +{ + struct netdev_hw_addr *ha; + struct nlattr *hw_list; + + if (!(ext_filter_mask & RTEXT_FILTER_MACLIST)) + return 0; + + if ((netdev_uc_count(dev) + netdev_mc_count(dev)) == 0) + return 0; + + hw_list = nla_nest_start(skb, IFLA_HW_ADDR_LIST); + if (!hw_list) + return -EMSGSIZE; + + /* Unicasts first */ + netdev_for_each_uc_addr(ha, dev) { + if (nla_put(skb, IFLA_ADDRESS, ETH_ALEN, ha->addr)) + goto nla_put_fail; + } + + /* Now multicasts */ + netdev_for_each_mc_addr(ha, dev) { + if (nla_put(skb, IFLA_ADDRESS, ETH_ALEN, ha->addr)) + goto nla_put_fail; + } + +nla_put_fail: + nla_nest_cancel(skb, hw_list); + return -EMSGSIZE; +} + static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, int type, u32 pid, u32 seq, u32 change, unsigned int flags, u32 ext_filter_mask) @@ -1044,6 +1092,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, } } + if (rtnl_hwaddr_fill(skb, dev, ext_filter_mask)) + goto nla_put_failure; + nla_nest_end(skb, af_spec); return nlmsg_end(skb, nlh); @@ -1128,6 +1179,8 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = { [IFLA_PROMISCUITY] = { .type = NLA_U32 }, [IFLA_NUM_TX_QUEUES] = { .type = NLA_U32 }, [IFLA_NUM_RX_QUEUES] = { .type = NLA_U32 }, + [IFLA_HW_ADDR_ADD] = { .type = NLA_BINARY, .len = ETH_ALEN }, + [IFLA_HW_ADDR_DEL] = { .type = NLA_BINARY, .len = ETH_ALEN }, }; EXPORT_SYMBOL(ifla_policy); @@ -1527,7 +1580,36 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, modified = 1; } } - err = 0; + + if (tb[IFLA_HW_ADDR_ADD]) { + const unsigned char *addr; + + addr = nla_data(tb[IFLA_HW_ADDR_ADD]); + + if (is_unicast_ether_addr(addr)) + err = dev_uc_add_excl(dev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_add_excl(dev, addr); + + if (err) + goto errout; + modified = 1; + } + + if (tb[IFLA_HW_ADDR_DEL]) { + const unsigned char *addr; + + addr = nla_data(tb[IFLA_HW_ADDR_DEL]); + + if (is_unicast_ether_addr(addr)) + err = dev_uc_del(dev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_del(dev, addr); + + if (err) + goto errout; + modified = 1; + } errout: if (err < 0 && modified) -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 19:44 ` Vlad Yasevich @ 2013-02-26 20:03 ` David Miller 2013-02-26 20:24 ` Vlad Yasevich 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2013-02-26 20:03 UTC (permalink / raw) To: vyasevic; +Cc: netdev From: Vlad Yasevich <vyasevic@redhat.com> Date: Tue, 26 Feb 2013 14:44:22 -0500 > David Miller wrote: >> No new ioctls please, extend the netlink interfaces as necessary >> instead. >> >> Thanks. > > So, something like below patch (compiled only) would be better in your opinion? > The one concern I have is that there is no way to probe for the availabilty > of this functionality, since netlink will just ignore unknown types and appear > to succeed. With an ioctl it is clear if the support is present. > > -- >8 -- > > > [RFC PATCH] rtnetlink: Add support for adding/removing additional hw addresses. > > Add an ability to add and remove HW addresses to the device > unicast and multicast address lists. Right now, we only have > an ioctl() to manage the multicast addresses and there is no > way the manage the unicast list. > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> This is a step in the right direction, and you're right that there is a difficulty in detecting whether support exists or not. I am so surprised that we've have ->set_rx_mode() support for multiple unicast MAC addresses in so many drivers all this time, yet no way outside of FDB to make use of it at all. Anyways, as per the feature detection issue, we could create a new RTM_* operation that returns a u32 feature flag mask. It's just one idea. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 20:03 ` David Miller @ 2013-02-26 20:24 ` Vlad Yasevich 2013-02-26 20:48 ` David Miller 2013-02-26 21:58 ` John Fastabend 0 siblings, 2 replies; 14+ messages in thread From: Vlad Yasevich @ 2013-02-26 20:24 UTC (permalink / raw) To: David Miller; +Cc: netdev On 02/26/2013 03:03 PM, David Miller wrote: > From: Vlad Yasevich <vyasevic@redhat.com> > Date: Tue, 26 Feb 2013 14:44:22 -0500 > >> David Miller wrote: >>> No new ioctls please, extend the netlink interfaces as necessary >>> instead. >>> >>> Thanks. >> >> So, something like below patch (compiled only) would be better in your opinion? >> The one concern I have is that there is no way to probe for the availabilty >> of this functionality, since netlink will just ignore unknown types and appear >> to succeed. With an ioctl it is clear if the support is present. >> >> -- >8 -- >> >> >> [RFC PATCH] rtnetlink: Add support for adding/removing additional hw addresses. >> >> Add an ability to add and remove HW addresses to the device >> unicast and multicast address lists. Right now, we only have >> an ioctl() to manage the multicast addresses and there is no >> way the manage the unicast list. >> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > > This is a step in the right direction, and you're right that there is > a difficulty in detecting whether support exists or not. > > I am so surprised that we've have ->set_rx_mode() support for multiple > unicast MAC addresses in so many drivers all this time, yet no way > outside of FDB to make use of it at all. And even that is not always available. In most drivers it requires module parameters or other explicit configuration steps. Meanwhile set_rx_mode() doesn't seem to depend on any of those and just does the right thing. For what I was trying to do ioctl() was a really easy way out for both kernel and user space implementation, so I gave is shot. -vlad > > Anyways, as per the feature detection issue, we could create a new > RTM_* operation that returns a u32 feature flag mask. It's just one > idea. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 20:24 ` Vlad Yasevich @ 2013-02-26 20:48 ` David Miller 2013-02-26 21:58 ` John Fastabend 1 sibling, 0 replies; 14+ messages in thread From: David Miller @ 2013-02-26 20:48 UTC (permalink / raw) To: vyasevic; +Cc: netdev From: Vlad Yasevich <vyasevic@redhat.com> Date: Tue, 26 Feb 2013 15:24:33 -0500 > For what I was trying to do ioctl() was a really easy way out for both > kernel and user space implementation, so I gave is shot. Look at it this way, you were going to have to add these config values to the rtnetlink LINK dumps anyways. :-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 20:24 ` Vlad Yasevich 2013-02-26 20:48 ` David Miller @ 2013-02-26 21:58 ` John Fastabend 2013-02-26 22:15 ` David Miller 1 sibling, 1 reply; 14+ messages in thread From: John Fastabend @ 2013-02-26 21:58 UTC (permalink / raw) To: vyasevic; +Cc: David Miller, netdev [...] >>> >>> >>> [RFC PATCH] rtnetlink: Add support for adding/removing additional hw >>> addresses. >>> >>> Add an ability to add and remove HW addresses to the device >>> unicast and multicast address lists. Right now, we only have >>> an ioctl() to manage the multicast addresses and there is no >>> way the manage the unicast list. >>> >>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >> >> This is a step in the right direction, and you're right that there is >> a difficulty in detecting whether support exists or not. >> >> I am so surprised that we've have ->set_rx_mode() support for multiple >> unicast MAC addresses in so many drivers all this time, yet no way >> outside of FDB to make use of it at all. > > And even that is not always available. In most drivers it requires > module parameters or other explicit configuration steps. Meanwhile > set_rx_mode() doesn't seem to depend on any of those and just does the > right thing. > > For what I was trying to do ioctl() was a really easy way out for both > kernel and user space implementation, so I gave is shot. > > -vlad > Don't we already support this with int (*ndo_fdb_add)(struct ndmsg *ndm, struct nlattr *tb[], struct net_device *dev, const unsigned char *addr, u16 flags); int (*ndo_fdb_del)(struct ndmsg *ndm, struct nlattr *tb[], struct net_device *dev, const unsigned char *addr); int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb, struct net_device *dev, int idx); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 21:58 ` John Fastabend @ 2013-02-26 22:15 ` David Miller 2013-02-27 2:07 ` John Fastabend 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2013-02-26 22:15 UTC (permalink / raw) To: john.r.fastabend; +Cc: vyasevic, netdev From: John Fastabend <john.r.fastabend@intel.com> Date: Tue, 26 Feb 2013 13:58:39 -0800 > [...] > >>>> >>>> >>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional hw >>>> addresses. >>>> >>>> Add an ability to add and remove HW addresses to the device >>>> unicast and multicast address lists. Right now, we only have >>>> an ioctl() to manage the multicast addresses and there is no >>>> way the manage the unicast list. >>>> >>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >>> >>> This is a step in the right direction, and you're right that there is >>> a difficulty in detecting whether support exists or not. >>> >>> I am so surprised that we've have ->set_rx_mode() support for multiple >>> unicast MAC addresses in so many drivers all this time, yet no way >>> outside of FDB to make use of it at all. >> >> And even that is not always available. In most drivers it requires >> module parameters or other explicit configuration steps. Meanwhile >> set_rx_mode() doesn't seem to depend on any of those and just does the >> right thing. >> >> For what I was trying to do ioctl() was a really easy way out for both >> kernel and user space implementation, so I gave is shot. >> >> -vlad >> > > Don't we already support this with The whole point is that these multiple-unicast-address configuration facilities are inaccessible without FDB, and there is no reason whatsoever for that. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-26 22:15 ` David Miller @ 2013-02-27 2:07 ` John Fastabend 2013-02-27 2:27 ` Vlad Yasevich 0 siblings, 1 reply; 14+ messages in thread From: John Fastabend @ 2013-02-27 2:07 UTC (permalink / raw) To: David Miller; +Cc: john.r.fastabend, vyasevic, netdev On Tue, Feb 26, 2013 at 05:15:03PM -0500, David Miller wrote: > From: John Fastabend <john.r.fastabend@intel.com> > Date: Tue, 26 Feb 2013 13:58:39 -0800 > > > [...] > > > >>>> > >>>> > >>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional hw > >>>> addresses. > >>>> > >>>> Add an ability to add and remove HW addresses to the device > >>>> unicast and multicast address lists. Right now, we only have > >>>> an ioctl() to manage the multicast addresses and there is no > >>>> way the manage the unicast list. > >>>> > >>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > >>> > >>> This is a step in the right direction, and you're right that there is > >>> a difficulty in detecting whether support exists or not. > >>> > >>> I am so surprised that we've have ->set_rx_mode() support for multiple > >>> unicast MAC addresses in so many drivers all this time, yet no way > >>> outside of FDB to make use of it at all. > >> > >> And even that is not always available. In most drivers it requires > >> module parameters or other explicit configuration steps. Meanwhile > >> set_rx_mode() doesn't seem to depend on any of those and just does the > >> right thing. > >> > >> For what I was trying to do ioctl() was a really easy way out for both > >> kernel and user space implementation, so I gave is shot. > >> > >> -vlad > >> > > > > Don't we already support this with > > The whole point is that these multiple-unicast-address configuration > facilities are inaccessible without FDB, and there is no reason > whatsoever for that. Yes I see now sorry I was behind the thread. We could just use the fdb hooks and add a dflt routine to handle the case where the netdev doesn't provide a specific ndo_op for us. This boils down to calling set_rx_mode anyways through this call chain, ndo_fdb_add dev_uc_add_excl __dev_set_rx_mode ndo_set_rx_mode As long as folks don't think this is too much of an abuse of the fdb hooks. Here's an example I only compile tested this so take it as an example only. It probably needs some cleanups and the dump routine needs some thought not sure you want to dump all MACs always. [RFC PATCH] net: fdb: generic set support for drivers without ndo_op If the driver does not support the ndo_op use the generic handler for it. This should work in the majority of cases. Eventually the fdb_dflt_add call gets translated into a __dev_set_rx_mode() call which should handle hardware support for filtering via the IFF_UNICAST_FLT flag. Namely IFF_UNICAST_FLT indicates if the hardware can do unicast address filtering. If no support is available the device is put into promisc mode. It looks like we likely also need IFF_MULTICAST_FLT and also tighten up how drivers handle multicast filters. But thats another patch. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- net/core/rtnetlink.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 74 insertions(+), 6 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d8aa20f..d1c6f71 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2049,6 +2049,37 @@ errout: rtnl_set_sk_err(net, RTNLGRP_NEIGH, err); } +/** + * ndo_dflt_fdb_add - default netdevice operation to add an FDB entry + */ +static int ndo_dflt_fdb_add(struct ndmsg *ndm, + struct nlattr *tb[], + struct net_device *dev, + const unsigned char *addr, + u16 flags) +{ + int err = -EINVAL; + + /* If aging addresses are supported device will need to + * implement its own handler for this. + */ + if (ndm->ndm_state && !(ndm->ndm_state & NUD_PERMANENT)) { + pr_info("%s: FDB only supports static addresses\n", dev->name); + return err; + } + + if (is_unicast_ether_addr(addr) || is_link_local_ether_addr(addr)) + err = dev_uc_add_excl(dev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_add_excl(dev, addr); + + /* Only return duplicate errors if NLM_F_EXCL is set */ + if (err == -EEXIST && !(flags & NLM_F_EXCL)) + err = 0; + + return err; +} + static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) { struct net *net = sock_net(skb->sk); @@ -2101,10 +2132,14 @@ static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) } /* Embedded bridge, macvlan, and any other device support */ - if ((ndm->ndm_flags & NTF_SELF) && dev->netdev_ops->ndo_fdb_add) { - err = dev->netdev_ops->ndo_fdb_add(ndm, tb, - dev, addr, - nlh->nlmsg_flags); + if ((ndm->ndm_flags & NTF_SELF)) { + if (dev->netdev_ops->ndo_fdb_add) + err = dev->netdev_ops->ndo_fdb_add(ndm, tb, + dev, addr, + nlh->nlmsg_flags); + else + err = ndo_dflt_fdb_add(ndm, tb, dev, addr, + nlh->nlmsg_flags); if (!err) { rtnl_fdb_notify(dev, addr, RTM_NEWNEIGH); @@ -2115,6 +2150,34 @@ out: return err; } +/** + * ndo_dflt_fdb_del - default netdevice operation to delete an FDB entry + */ +static int ndo_dflt_fdb_del(struct ndmsg *ndm, + struct nlattr *tb[], + struct net_device *dev, + const unsigned char *addr) +{ + int err = -EOPNOTSUPP; + + /* If aging addresses are supported device will need to + * implement its own handler for this. + */ + if (ndm->ndm_state & NUD_PERMANENT) { + pr_info("%s: FDB only supports static addresses\n", dev->name); + return -EINVAL; + } + + if (is_unicast_ether_addr(addr)) + err = dev_uc_del(dev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_del(dev, addr); + else + err = -EINVAL; + + return err; +} + static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) { struct net *net = sock_net(skb->sk); @@ -2172,8 +2235,11 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) } /* Embedded bridge, macvlan, and any other device support */ - if ((ndm->ndm_flags & NTF_SELF) && dev->netdev_ops->ndo_fdb_del) { - err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr); + if (ndm->ndm_flags & NTF_SELF) { + if (dev->netdev_ops->ndo_fdb_del) + err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr); + else + err = ndo_dflt_fdb_del(ndm, tb, dev, addr); if (!err) { rtnl_fdb_notify(dev, addr, RTM_DELNEIGH); @@ -2258,6 +2324,8 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb) if (dev->netdev_ops->ndo_fdb_dump) idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, idx); + else + ndo_dflt_fdb_dump(skb, cb, dev, idx); } rcu_read_unlock(); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-27 2:07 ` John Fastabend @ 2013-02-27 2:27 ` Vlad Yasevich 2013-02-27 5:01 ` John Fastabend 0 siblings, 1 reply; 14+ messages in thread From: Vlad Yasevich @ 2013-02-27 2:27 UTC (permalink / raw) To: John Fastabend; +Cc: David Miller, john.r.fastabend, netdev On 02/26/2013 09:07 PM, John Fastabend wrote: > On Tue, Feb 26, 2013 at 05:15:03PM -0500, David Miller wrote: >> From: John Fastabend <john.r.fastabend@intel.com> >> Date: Tue, 26 Feb 2013 13:58:39 -0800 >> >>> [...] >>> >>>>>> >>>>>> >>>>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional hw >>>>>> addresses. >>>>>> >>>>>> Add an ability to add and remove HW addresses to the device >>>>>> unicast and multicast address lists. Right now, we only have >>>>>> an ioctl() to manage the multicast addresses and there is no >>>>>> way the manage the unicast list. >>>>>> >>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >>>>> >>>>> This is a step in the right direction, and you're right that there is >>>>> a difficulty in detecting whether support exists or not. >>>>> >>>>> I am so surprised that we've have ->set_rx_mode() support for multiple >>>>> unicast MAC addresses in so many drivers all this time, yet no way >>>>> outside of FDB to make use of it at all. >>>> >>>> And even that is not always available. In most drivers it requires >>>> module parameters or other explicit configuration steps. Meanwhile >>>> set_rx_mode() doesn't seem to depend on any of those and just does the >>>> right thing. >>>> >>>> For what I was trying to do ioctl() was a really easy way out for both >>>> kernel and user space implementation, so I gave is shot. >>>> >>>> -vlad >>>> >>> >>> Don't we already support this with >> >> The whole point is that these multiple-unicast-address configuration >> facilities are inaccessible without FDB, and there is no reason >> whatsoever for that. > > Yes I see now sorry I was behind the thread. > > We could just use the fdb hooks and add a dflt routine to handle the > case where the netdev doesn't provide a specific ndo_op for us. This > boils down to calling set_rx_mode anyways through this call chain, > > ndo_fdb_add > dev_uc_add_excl > __dev_set_rx_mode > ndo_set_rx_mode > > As long as folks don't think this is too much of an abuse of the fdb > hooks. Here's an example I only compile tested this so take it as > an example only. It probably needs some cleanups and the dump routine > needs some thought not sure you want to dump all MACs always. I thought about doing, but this becomes broken on the drivers that support ndo_fdb_* functions. Those drivers mainly require additional configuration that is not necessary for dev_uc_add_excl() to work. For example, ixgbe and melanox requires VF to be on, qlogic needs a module parameter, macvlan has to be in passthrough. However, ndo_set_rx_mode() in most cases doesn't care about those settings and just works. So fdb will not always work even if the driver has proper support for IFF_UNICAST_FLT. -vlad > > > [RFC PATCH] net: fdb: generic set support for drivers without ndo_op > > If the driver does not support the ndo_op use the generic > handler for it. This should work in the majority of cases. > Eventually the fdb_dflt_add call gets translated into a > __dev_set_rx_mode() call which should handle hardware > support for filtering via the IFF_UNICAST_FLT flag. > > Namely IFF_UNICAST_FLT indicates if the hardware can do > unicast address filtering. If no support is available > the device is put into promisc mode. > > It looks like we likely also need IFF_MULTICAST_FLT and > also tighten up how drivers handle multicast filters. But > thats another patch. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > net/core/rtnetlink.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 74 insertions(+), 6 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index d8aa20f..d1c6f71 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -2049,6 +2049,37 @@ errout: > rtnl_set_sk_err(net, RTNLGRP_NEIGH, err); > } > > +/** > + * ndo_dflt_fdb_add - default netdevice operation to add an FDB entry > + */ > +static int ndo_dflt_fdb_add(struct ndmsg *ndm, > + struct nlattr *tb[], > + struct net_device *dev, > + const unsigned char *addr, > + u16 flags) > +{ > + int err = -EINVAL; > + > + /* If aging addresses are supported device will need to > + * implement its own handler for this. > + */ > + if (ndm->ndm_state && !(ndm->ndm_state & NUD_PERMANENT)) { > + pr_info("%s: FDB only supports static addresses\n", dev->name); > + return err; > + } > + > + if (is_unicast_ether_addr(addr) || is_link_local_ether_addr(addr)) > + err = dev_uc_add_excl(dev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_add_excl(dev, addr); > + > + /* Only return duplicate errors if NLM_F_EXCL is set */ > + if (err == -EEXIST && !(flags & NLM_F_EXCL)) > + err = 0; > + > + return err; > +} > + > static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) > { > struct net *net = sock_net(skb->sk); > @@ -2101,10 +2132,14 @@ static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) > } > > /* Embedded bridge, macvlan, and any other device support */ > - if ((ndm->ndm_flags & NTF_SELF) && dev->netdev_ops->ndo_fdb_add) { > - err = dev->netdev_ops->ndo_fdb_add(ndm, tb, > - dev, addr, > - nlh->nlmsg_flags); > + if ((ndm->ndm_flags & NTF_SELF)) { > + if (dev->netdev_ops->ndo_fdb_add) > + err = dev->netdev_ops->ndo_fdb_add(ndm, tb, > + dev, addr, > + nlh->nlmsg_flags); > + else > + err = ndo_dflt_fdb_add(ndm, tb, dev, addr, > + nlh->nlmsg_flags); > > if (!err) { > rtnl_fdb_notify(dev, addr, RTM_NEWNEIGH); > @@ -2115,6 +2150,34 @@ out: > return err; > } > > +/** > + * ndo_dflt_fdb_del - default netdevice operation to delete an FDB entry > + */ > +static int ndo_dflt_fdb_del(struct ndmsg *ndm, > + struct nlattr *tb[], > + struct net_device *dev, > + const unsigned char *addr) > +{ > + int err = -EOPNOTSUPP; > + > + /* If aging addresses are supported device will need to > + * implement its own handler for this. > + */ > + if (ndm->ndm_state & NUD_PERMANENT) { > + pr_info("%s: FDB only supports static addresses\n", dev->name); > + return -EINVAL; > + } > + > + if (is_unicast_ether_addr(addr)) > + err = dev_uc_del(dev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_del(dev, addr); > + else > + err = -EINVAL; > + > + return err; > +} > + > static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) > { > struct net *net = sock_net(skb->sk); > @@ -2172,8 +2235,11 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) > } > > /* Embedded bridge, macvlan, and any other device support */ > - if ((ndm->ndm_flags & NTF_SELF) && dev->netdev_ops->ndo_fdb_del) { > - err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr); > + if (ndm->ndm_flags & NTF_SELF) { > + if (dev->netdev_ops->ndo_fdb_del) > + err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr); > + else > + err = ndo_dflt_fdb_del(ndm, tb, dev, addr); > > if (!err) { > rtnl_fdb_notify(dev, addr, RTM_DELNEIGH); > @@ -2258,6 +2324,8 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb) > > if (dev->netdev_ops->ndo_fdb_dump) > idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, idx); > + else > + ndo_dflt_fdb_dump(skb, cb, dev, idx); > } > rcu_read_unlock(); > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-27 2:27 ` Vlad Yasevich @ 2013-02-27 5:01 ` John Fastabend 2013-02-27 5:42 ` Jitendra Kalsaria 0 siblings, 1 reply; 14+ messages in thread From: John Fastabend @ 2013-02-27 5:01 UTC (permalink / raw) To: vyasevic; +Cc: John Fastabend, David Miller, netdev On 2/26/2013 6:27 PM, Vlad Yasevich wrote: > On 02/26/2013 09:07 PM, John Fastabend wrote: >> On Tue, Feb 26, 2013 at 05:15:03PM -0500, David Miller wrote: >>> From: John Fastabend <john.r.fastabend@intel.com> >>> Date: Tue, 26 Feb 2013 13:58:39 -0800 >>> >>>> [...] >>>> >>>>>>> >>>>>>> >>>>>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional hw >>>>>>> addresses. >>>>>>> >>>>>>> Add an ability to add and remove HW addresses to the device >>>>>>> unicast and multicast address lists. Right now, we only have >>>>>>> an ioctl() to manage the multicast addresses and there is no >>>>>>> way the manage the unicast list. >>>>>>> >>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >>>>>> >>>>>> This is a step in the right direction, and you're right that there is >>>>>> a difficulty in detecting whether support exists or not. >>>>>> >>>>>> I am so surprised that we've have ->set_rx_mode() support for >>>>>> multiple >>>>>> unicast MAC addresses in so many drivers all this time, yet no way >>>>>> outside of FDB to make use of it at all. >>>>> >>>>> And even that is not always available. In most drivers it requires >>>>> module parameters or other explicit configuration steps. Meanwhile >>>>> set_rx_mode() doesn't seem to depend on any of those and just does the >>>>> right thing. >>>>> >>>>> For what I was trying to do ioctl() was a really easy way out for both >>>>> kernel and user space implementation, so I gave is shot. >>>>> >>>>> -vlad >>>>> >>>> >>>> Don't we already support this with >>> >>> The whole point is that these multiple-unicast-address configuration >>> facilities are inaccessible without FDB, and there is no reason >>> whatsoever for that. >> >> Yes I see now sorry I was behind the thread. >> >> We could just use the fdb hooks and add a dflt routine to handle the >> case where the netdev doesn't provide a specific ndo_op for us. This >> boils down to calling set_rx_mode anyways through this call chain, >> >> ndo_fdb_add >> dev_uc_add_excl >> __dev_set_rx_mode >> ndo_set_rx_mode >> >> As long as folks don't think this is too much of an abuse of the fdb >> hooks. Here's an example I only compile tested this so take it as >> an example only. It probably needs some cleanups and the dump routine >> needs some thought not sure you want to dump all MACs always. > > I thought about doing, but this becomes broken on the drivers that > support ndo_fdb_* functions. Those drivers mainly require additional > configuration that is not necessary for dev_uc_add_excl() to work. > > For example, ixgbe and melanox requires VF to be on, qlogic needs a > module parameter, macvlan has to be in passthrough. However, > ndo_set_rx_mode() in most cases doesn't care about those settings and > just works. > The ixgbe piece could just use the dflt routines I put below. The SR-IOV check is only there because when I wrote that I didn't consider adding additional addresses without VFs. This was a poor example. The mlx4 card is checking if the device is a slave or master before adding/removing addresses. My guess is this is the same type of check as ixgbe and would work just fine without it. I'm not sure macvlan supports unicast hw addresses either way but there is no reason we couldn't. The idea was we would come back and write it later. Like many things I haven't got to it yet. Calling set_rx_mode() on qlcnic doesn't look to help you at all either it appears to ignore the uc_list either way. is that all the callers? looks like it. > So fdb will not always work even if the driver has proper support for > IFF_UNICAST_FLT. > The fdb could work if we fix the drivers it seems to me most the cases where it wouldn't work are just lack of foresight or the code isn't there yet. I think it might be valuable to have a single API for both use cases. .John > -vlad > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-27 5:01 ` John Fastabend @ 2013-02-27 5:42 ` Jitendra Kalsaria 2013-02-27 7:34 ` John Fastabend 2013-02-27 15:27 ` Vlad Yasevich 0 siblings, 2 replies; 14+ messages in thread From: Jitendra Kalsaria @ 2013-02-27 5:42 UTC (permalink / raw) To: John Fastabend, vyasevic@redhat.com; +Cc: John Fastabend, David Miller, netdev On 2/26/13 9:01 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: >On 2/26/2013 6:27 PM, Vlad Yasevich wrote: >> On 02/26/2013 09:07 PM, John Fastabend wrote: >>> On Tue, Feb 26, 2013 at 05:15:03PM -0500, David Miller wrote: >>>> From: John Fastabend <john.r.fastabend@intel.com> >>>> Date: Tue, 26 Feb 2013 13:58:39 -0800 >>>> >>>>> [...] >>>>> >>>>>>>> >>>>>>>> >>>>>>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional >>>>>>>>hw >>>>>>>> addresses. >>>>>>>> >>>>>>>> Add an ability to add and remove HW addresses to the device >>>>>>>> unicast and multicast address lists. Right now, we only have >>>>>>>> an ioctl() to manage the multicast addresses and there is no >>>>>>>> way the manage the unicast list. >>>>>>>> >>>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >>>>>>> >>>>>>> This is a step in the right direction, and you're right that there >>>>>>>is >>>>>>> a difficulty in detecting whether support exists or not. >>>>>>> >>>>>>> I am so surprised that we've have ->set_rx_mode() support for >>>>>>> multiple >>>>>>> unicast MAC addresses in so many drivers all this time, yet no way >>>>>>> outside of FDB to make use of it at all. >>>>>> >>>>>> And even that is not always available. In most drivers it requires >>>>>> module parameters or other explicit configuration steps. Meanwhile >>>>>> set_rx_mode() doesn't seem to depend on any of those and just does >>>>>>the >>>>>> right thing. >>>>>> >>>>>> For what I was trying to do ioctl() was a really easy way out for >>>>>>both >>>>>> kernel and user space implementation, so I gave is shot. >>>>>> >>>>>> -vlad >>>>>> >>>>> >>>>> Don't we already support this with >>>> >>>> The whole point is that these multiple-unicast-address configuration >>>> facilities are inaccessible without FDB, and there is no reason >>>> whatsoever for that. >>> >>> Yes I see now sorry I was behind the thread. >>> >>> We could just use the fdb hooks and add a dflt routine to handle the >>> case where the netdev doesn't provide a specific ndo_op for us. This >>> boils down to calling set_rx_mode anyways through this call chain, >>> >>> ndo_fdb_add >>> dev_uc_add_excl >>> __dev_set_rx_mode >>> ndo_set_rx_mode >>> >>> As long as folks don't think this is too much of an abuse of the fdb >>> hooks. Here's an example I only compile tested this so take it as >>> an example only. It probably needs some cleanups and the dump routine >>> needs some thought not sure you want to dump all MACs always. >> >> I thought about doing, but this becomes broken on the drivers that >> support ndo_fdb_* functions. Those drivers mainly require additional >> configuration that is not necessary for dev_uc_add_excl() to work. >> >> For example, ixgbe and melanox requires VF to be on, qlogic needs a >> module parameter, macvlan has to be in passthrough. However, >> ndo_set_rx_mode() in most cases doesn't care about those settings and >> just works. >> > >The ixgbe piece could just use the dflt routines I put below. The >SR-IOV check is only there because when I wrote that I didn't consider >adding additional addresses without VFs. This was a poor example. > >The mlx4 card is checking if the device is a slave or master before >adding/removing addresses. My guess is this is the same type of check >as ixgbe and would work just fine without it. > >I'm not sure macvlan supports unicast hw addresses either way but >there is no reason we couldn't. The idea was we would come back and >write it later. Like many things I haven't got to it yet. > >Calling set_rx_mode() on qlcnic doesn't look to help you at all either >it appears to ignore the uc_list either way. Even though we use module parameter for fdb but it would be good it have set_rx_mode(). We don't ignore uc_list but it never called set_rx_mode() when I use dev_uc_add_excl when added support and thought that's how it might designed. -Jiten > >is that all the callers? looks like it. > >> So fdb will not always work even if the driver has proper support for >> IFF_UNICAST_FLT. >> > >The fdb could work if we fix the drivers it seems to me most >the cases where it wouldn't work are just lack of foresight or the code >isn't there yet. I think it might be valuable to have a single API for >both use cases. > >.John > >> -vlad >> >-- >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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-27 5:42 ` Jitendra Kalsaria @ 2013-02-27 7:34 ` John Fastabend 2013-02-27 15:27 ` Vlad Yasevich 1 sibling, 0 replies; 14+ messages in thread From: John Fastabend @ 2013-02-27 7:34 UTC (permalink / raw) To: Jitendra Kalsaria Cc: John Fastabend, vyasevic@redhat.com, David Miller, netdev On 02/26/2013 09:42 PM, Jitendra Kalsaria wrote: > > > On 2/26/13 9:01 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: > >> On 2/26/2013 6:27 PM, Vlad Yasevich wrote: >>> On 02/26/2013 09:07 PM, John Fastabend wrote: >>>> On Tue, Feb 26, 2013 at 05:15:03PM -0500, David Miller wrote: >>>>> From: John Fastabend <john.r.fastabend@intel.com> >>>>> Date: Tue, 26 Feb 2013 13:58:39 -0800 >>>>> >>>>>> [...] >>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional >>>>>>>>> hw >>>>>>>>> addresses. >>>>>>>>> >>>>>>>>> Add an ability to add and remove HW addresses to the device >>>>>>>>> unicast and multicast address lists. Right now, we only have >>>>>>>>> an ioctl() to manage the multicast addresses and there is no >>>>>>>>> way the manage the unicast list. >>>>>>>>> >>>>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >>>>>>>> >>>>>>>> This is a step in the right direction, and you're right that there >>>>>>>> is >>>>>>>> a difficulty in detecting whether support exists or not. >>>>>>>> >>>>>>>> I am so surprised that we've have ->set_rx_mode() support for >>>>>>>> multiple >>>>>>>> unicast MAC addresses in so many drivers all this time, yet no way >>>>>>>> outside of FDB to make use of it at all. >>>>>>> >>>>>>> And even that is not always available. In most drivers it requires >>>>>>> module parameters or other explicit configuration steps. Meanwhile >>>>>>> set_rx_mode() doesn't seem to depend on any of those and just does >>>>>>> the >>>>>>> right thing. >>>>>>> >>>>>>> For what I was trying to do ioctl() was a really easy way out for >>>>>>> both >>>>>>> kernel and user space implementation, so I gave is shot. >>>>>>> >>>>>>> -vlad >>>>>>> >>>>>> >>>>>> Don't we already support this with >>>>> >>>>> The whole point is that these multiple-unicast-address configuration >>>>> facilities are inaccessible without FDB, and there is no reason >>>>> whatsoever for that. >>>> >>>> Yes I see now sorry I was behind the thread. >>>> >>>> We could just use the fdb hooks and add a dflt routine to handle the >>>> case where the netdev doesn't provide a specific ndo_op for us. This >>>> boils down to calling set_rx_mode anyways through this call chain, >>>> >>>> ndo_fdb_add >>>> dev_uc_add_excl >>>> __dev_set_rx_mode >>>> ndo_set_rx_mode >>>> >>>> As long as folks don't think this is too much of an abuse of the fdb >>>> hooks. Here's an example I only compile tested this so take it as >>>> an example only. It probably needs some cleanups and the dump routine >>>> needs some thought not sure you want to dump all MACs always. >>> >>> I thought about doing, but this becomes broken on the drivers that >>> support ndo_fdb_* functions. Those drivers mainly require additional >>> configuration that is not necessary for dev_uc_add_excl() to work. >>> >>> For example, ixgbe and melanox requires VF to be on, qlogic needs a >>> module parameter, macvlan has to be in passthrough. However, >>> ndo_set_rx_mode() in most cases doesn't care about those settings and >>> just works. >>> >> >> The ixgbe piece could just use the dflt routines I put below. The >> SR-IOV check is only there because when I wrote that I didn't consider >> adding additional addresses without VFs. This was a poor example. >> >> The mlx4 card is checking if the device is a slave or master before >> adding/removing addresses. My guess is this is the same type of check >> as ixgbe and would work just fine without it. >> >> I'm not sure macvlan supports unicast hw addresses either way but >> there is no reason we couldn't. The idea was we would come back and >> write it later. Like many things I haven't got to it yet. >> >> Calling set_rx_mode() on qlcnic doesn't look to help you at all either >> it appears to ignore the uc_list either way. > > Even though we use module parameter for fdb but it would be good it have > set_rx_mode(). > We don't ignore uc_list but it never called set_rx_mode() when I use > dev_uc_add_excl when added support and thought that's how it might > designed. > > -Jiten >> Hopefully not too off topic but you probably should use dev_uc_add_excl in fdb_add and then fix your set_rx_mode() handler to configure the uc_list correctly. Not sure what happens today when a unicast addr is added to the list for example with macvlan looks like a bug to me. .John -- John Fastabend Intel Corporation ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses 2013-02-27 5:42 ` Jitendra Kalsaria 2013-02-27 7:34 ` John Fastabend @ 2013-02-27 15:27 ` Vlad Yasevich 1 sibling, 0 replies; 14+ messages in thread From: Vlad Yasevich @ 2013-02-27 15:27 UTC (permalink / raw) To: Jitendra Kalsaria; +Cc: John Fastabend, John Fastabend, David Miller, netdev On 02/27/2013 12:42 AM, Jitendra Kalsaria wrote: > > > On 2/26/13 9:01 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: > >> On 2/26/2013 6:27 PM, Vlad Yasevich wrote: >>> On 02/26/2013 09:07 PM, John Fastabend wrote: >>>> On Tue, Feb 26, 2013 at 05:15:03PM -0500, David Miller wrote: >>>>> From: John Fastabend <john.r.fastabend@intel.com> >>>>> Date: Tue, 26 Feb 2013 13:58:39 -0800 >>>>> >>>>>> [...] >>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional >>>>>>>>> hw >>>>>>>>> addresses. >>>>>>>>> >>>>>>>>> Add an ability to add and remove HW addresses to the device >>>>>>>>> unicast and multicast address lists. Right now, we only have >>>>>>>>> an ioctl() to manage the multicast addresses and there is no >>>>>>>>> way the manage the unicast list. >>>>>>>>> >>>>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >>>>>>>> >>>>>>>> This is a step in the right direction, and you're right that there >>>>>>>> is >>>>>>>> a difficulty in detecting whether support exists or not. >>>>>>>> >>>>>>>> I am so surprised that we've have ->set_rx_mode() support for >>>>>>>> multiple >>>>>>>> unicast MAC addresses in so many drivers all this time, yet no way >>>>>>>> outside of FDB to make use of it at all. >>>>>>> >>>>>>> And even that is not always available. In most drivers it requires >>>>>>> module parameters or other explicit configuration steps. Meanwhile >>>>>>> set_rx_mode() doesn't seem to depend on any of those and just does >>>>>>> the >>>>>>> right thing. >>>>>>> >>>>>>> For what I was trying to do ioctl() was a really easy way out for >>>>>>> both >>>>>>> kernel and user space implementation, so I gave is shot. >>>>>>> >>>>>>> -vlad >>>>>>> >>>>>> >>>>>> Don't we already support this with >>>>> >>>>> The whole point is that these multiple-unicast-address configuration >>>>> facilities are inaccessible without FDB, and there is no reason >>>>> whatsoever for that. >>>> >>>> Yes I see now sorry I was behind the thread. >>>> >>>> We could just use the fdb hooks and add a dflt routine to handle the >>>> case where the netdev doesn't provide a specific ndo_op for us. This >>>> boils down to calling set_rx_mode anyways through this call chain, >>>> >>>> ndo_fdb_add >>>> dev_uc_add_excl >>>> __dev_set_rx_mode >>>> ndo_set_rx_mode >>>> >>>> As long as folks don't think this is too much of an abuse of the fdb >>>> hooks. Here's an example I only compile tested this so take it as >>>> an example only. It probably needs some cleanups and the dump routine >>>> needs some thought not sure you want to dump all MACs always. >>> >>> I thought about doing, but this becomes broken on the drivers that >>> support ndo_fdb_* functions. Those drivers mainly require additional >>> configuration that is not necessary for dev_uc_add_excl() to work. >>> >>> For example, ixgbe and melanox requires VF to be on, qlogic needs a >>> module parameter, macvlan has to be in passthrough. However, >>> ndo_set_rx_mode() in most cases doesn't care about those settings and >>> just works. >>> >> >> The ixgbe piece could just use the dflt routines I put below. The >> SR-IOV check is only there because when I wrote that I didn't consider >> adding additional addresses without VFs. This was a poor example. >> >> The mlx4 card is checking if the device is a slave or master before >> adding/removing addresses. My guess is this is the same type of check >> as ixgbe and would work just fine without it. >> >> I'm not sure macvlan supports unicast hw addresses either way but >> there is no reason we couldn't. The idea was we would come back and >> write it later. Like many things I haven't got to it yet. >> >> Calling set_rx_mode() on qlcnic doesn't look to help you at all either >> it appears to ignore the uc_list either way. > > Even though we use module parameter for fdb but it would be good it have > set_rx_mode(). > We don't ignore uc_list but it never called set_rx_mode() when I use > dev_uc_add_excl when added support and thought that's how it might > designed. I doesn't look like you ever call dev_uc_add_excl(). Also, since the driver doesn't set IFF_UNICAST_FLT, anyone who does call that (macvlan for instance) will end up turning promisc on. -vlad > > -Jiten >> >> is that all the callers? looks like it. >> >>> So fdb will not always work even if the driver has proper support for >>> IFF_UNICAST_FLT. >>> >> >> The fdb could work if we fix the drivers it seems to me most >> the cases where it wouldn't work are just lack of foresight or the code >> isn't there yet. I think it might be valuable to have a single API for >> both use cases. >> >> .John >> >>> -vlad >>> >> -- >> 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 >> > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-02-27 15:28 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-26 16:59 [RFC PATCH] core: Add ioctls to control device unicast hw addresses Vlad Yasevich 2013-02-26 17:43 ` David Miller 2013-02-26 19:44 ` Vlad Yasevich 2013-02-26 20:03 ` David Miller 2013-02-26 20:24 ` Vlad Yasevich 2013-02-26 20:48 ` David Miller 2013-02-26 21:58 ` John Fastabend 2013-02-26 22:15 ` David Miller 2013-02-27 2:07 ` John Fastabend 2013-02-27 2:27 ` Vlad Yasevich 2013-02-27 5:01 ` John Fastabend 2013-02-27 5:42 ` Jitendra Kalsaria 2013-02-27 7:34 ` John Fastabend 2013-02-27 15:27 ` Vlad Yasevich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).