Messages in this thread | | | Date | Thu, 1 Aug 2013 22:43:46 +0300 | From | "Michael S. Tsirkin" <> | Subject | Re: [PATCH v2] macvlan: validate flags |
| |
On Thu, Aug 01, 2013 at 12:34:24PM -0700, John Fastabend wrote: > On 8/1/2013 12:19 PM, Michael S. Tsirkin wrote: > >On Thu, Aug 01, 2013 at 10:24:19AM -0700, John Fastabend wrote: > >>On 8/1/2013 9:09 AM, Michael S. Tsirkin wrote: > >>>commit df8ef8f3aaa6692970a436204c4429210addb23a > >>> macvlan: add FDB bridge ops and macvlan flags > >>>added a flags field to macvlan, which can be > >>>controlled from userspace. > >>>The idea is to make the interface future-proof > >>>so we can add flags and not new fields. > >>> > >>>However, flags value isn't validated, as a result, > >>>userspace can't detect which flags are supported. > >>> > >>>Cc: "David S. Miller" <davem@davemloft.net> > >>>Cc: John Fastabend <john.r.fastabend@intel.com> > >>>Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>>--- > >>> > >>>Changes from v1: > >>> tweaked commit message > >>> no code changes > >>> > >>>Please consider this patch for -stable. > >>> > >>>The idea is by the time we add more flags, > >>>everyone has updated to a kernel that > >>>detects errors, so userspace will be able > >>>to detect supported flags cleanly. > >>> > >> > >>Agreed and because we haven't added more flags yet this shouldn't > >>break uapi. Thanks for catching this. > >> > >>> > >>> drivers/net/macvlan.c | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >> > >>By the same logic should we also add the check to macvlan_changelink()? > > > >I'm not sure what do you mean "By the same logic" - > >macvlan_changelink is static unlike macvlan_common_newlink > >which is exported to modules. > > "By the same logic" I only meant to allow userspace to cleanly detect > supported flags even in the changelink case. > > >So why isn't macvlan_validate sufficient for macvlan_changelink? > > It is you are correct. > > > > >>>diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > >>>index 18373b6..8445a94 100644 > >>>--- a/drivers/net/macvlan.c > >>>+++ b/drivers/net/macvlan.c > >>>@@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[]) > >>> return -EADDRNOTAVAIL; > >>> } > >>> > >>>+ if (data && data[IFLA_MACVLAN_FLAGS] && > >>>+ nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC) > >>>+ return -EINVAL; > >>>+ > >>> if (data && data[IFLA_MACVLAN_MODE]) { > >>> switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) { > >>> case MACVLAN_MODE_PRIVATE: > >>>@@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev, > >>> if (data && data[IFLA_MACVLAN_FLAGS]) > >>> vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]); > >>> > >>>+ if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC) > >>>+ return -EINVAL; > >>>+ > >> > >>Is there really a case where newlink is called without first calling > >>validate? I don't think there is so the snippet here in newlink could > >>be dropped. > >> > >>Thanks, > >>John > > > >It seems so - macvtap_newlink calls macvlan_common_newlink. > >macvtap does not seem to have .validate. > > > > but it calls macvlan_link_register() from macvtap_init which sets > up the validate ops, > > int macvlan_link_register(struct rtnl_link_ops *ops) > { > /* common fields */ > ops->priv_size = sizeof(struct macvlan_dev); > ops->validate = macvlan_validate
I see. I'll drop this second chunk in the patch, thanks for catching this.
-- MST
| |