[PATCH net v2] openvswitch: properly refcount vport-vxlan module

[PATCH net v2] openvswitch: properly refcount vport-vxlan module

[Paolo Abeni]

After 614732eaa12d, no refcount is maintained for the vport-vxlan module.
This allows the userspace to remove such module while vport-vxlan
devices still exist, which leads to later oops.

v1 -> v2:
 - move vport 'owner' initialization in ovs_vport_ops_register()
   and make such function a macro

Fixes: 614732eaa12d ("openvswitch: Use regular VXLAN net_device device")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/openvswitch/vport-geneve.c | 1 -
 net/openvswitch/vport-gre.c    | 1 -
 net/openvswitch/vport.c        | 4 ++--
 net/openvswitch/vport.h        | 8 +++++++-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/openvswitch/vport-geneve.c b/net/openvswitch/vport-geneve.c
index efb736b..e41cd12 100644
--- a/net/openvswitch/vport-geneve.c
+++ b/net/openvswitch/vport-geneve.c
@@ -117,7 +117,6 @@ static struct vport_ops ovs_geneve_vport_ops = {
 	.destroy	= ovs_netdev_tunnel_destroy,
 	.get_options	= geneve_get_options,
 	.send		= dev_queue_xmit,
-	.owner          = THIS_MODULE,
 };
 
 static int __init ovs_geneve_tnl_init(void)
diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
index c3257d7..7f8897f 100644
--- a/net/openvswitch/vport-gre.c
+++ b/net/openvswitch/vport-gre.c
@@ -89,7 +89,6 @@ static struct vport_ops ovs_gre_vport_ops = {
 	.create		= gre_create,
 	.send		= dev_queue_xmit,
 	.destroy	= ovs_netdev_tunnel_destroy,
-	.owner		= THIS_MODULE,
 };
 
 static int __init ovs_gre_tnl_init(void)
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index e194c10a..31cbc8c 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -71,7 +71,7 @@ static struct hlist_head *hash_bucket(const struct net *net, const char *name)
 	return &dev_table[hash & (VPORT_HASH_BUCKETS - 1)];
 }
 
-int ovs_vport_ops_register(struct vport_ops *ops)
+int __ovs_vport_ops_register(struct vport_ops *ops)
 {
 	int err = -EEXIST;
 	struct vport_ops *o;
@@ -87,7 +87,7 @@ errout:
 	ovs_unlock();
 	return err;
 }
-EXPORT_SYMBOL_GPL(ovs_vport_ops_register);
+EXPORT_SYMBOL_GPL(__ovs_vport_ops_register);
 
 void ovs_vport_ops_unregister(struct vport_ops *ops)
 {
diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
index bdfd82a..8ea3a96 100644
--- a/net/openvswitch/vport.h
+++ b/net/openvswitch/vport.h
@@ -196,7 +196,13 @@ static inline const char *ovs_vport_name(struct vport *vport)
 	return vport->dev->name;
 }
 
-int ovs_vport_ops_register(struct vport_ops *ops);
+int __ovs_vport_ops_register(struct vport_ops *ops);
+#define ovs_vport_ops_register(ops)		\
+	({					\
+		(ops)->owner = THIS_MODULE;	\
+		__ovs_vport_ops_register(ops);	\
+	})
+
 void ovs_vport_ops_unregister(struct vport_ops *ops);
 
 static inline struct rtable *ovs_tunnel_route_lookup(struct net *net,
-- 
1.8.3.1

Re: [PATCH net v2] openvswitch: properly refcount vport-vxlan module

[Hannes Frederic Sowa]

Hello Paolo,

On Mon, Nov 30, 2015, at 12:31, Paolo Abeni wrote:
> After 614732eaa12d, no refcount is maintained for the vport-vxlan module.
> This allows the userspace to remove such module while vport-vxlan
> devices still exist, which leads to later oops.
> 
> v1 -> v2:
>  - move vport 'owner' initialization in ovs_vport_ops_register()
>    and make such function a macro
> 
> Fixes: 614732eaa12d ("openvswitch: Use regular VXLAN net_device device")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/openvswitch/vport-geneve.c | 1 -
>  net/openvswitch/vport-gre.c    | 1 -
>  net/openvswitch/vport.c        | 4 ++--
>  net/openvswitch/vport.h        | 8 +++++++-
>  4 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/openvswitch/vport-geneve.c
> b/net/openvswitch/vport-geneve.c
> index efb736b..e41cd12 100644
> --- a/net/openvswitch/vport-geneve.c
> +++ b/net/openvswitch/vport-geneve.c
> @@ -117,7 +117,6 @@ static struct vport_ops ovs_geneve_vport_ops = {
>  	.destroy	= ovs_netdev_tunnel_destroy,
>  	.get_options	= geneve_get_options,
>  	.send		= dev_queue_xmit,
> -       .owner          = THIS_MODULE,
>  };
>  
>  static int __init ovs_geneve_tnl_init(void)
> diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> index c3257d7..7f8897f 100644
> --- a/net/openvswitch/vport-gre.c
> +++ b/net/openvswitch/vport-gre.c
> @@ -89,7 +89,6 @@ static struct vport_ops ovs_gre_vport_ops = {
>  	.create		= gre_create,
>  	.send		= dev_queue_xmit,
>  	.destroy	= ovs_netdev_tunnel_destroy,
> -       .owner          = THIS_MODULE,
>  };
>  
>  static int __init ovs_gre_tnl_init(void)
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index e194c10a..31cbc8c 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -71,7 +71,7 @@ static struct hlist_head *hash_bucket(const struct net
> *net, const char *name)
>  	return &dev_table[hash & (VPORT_HASH_BUCKETS - 1)];
>  }
>  
> -int ovs_vport_ops_register(struct vport_ops *ops)
> +int __ovs_vport_ops_register(struct vport_ops *ops)
>  {
>  	int err = -EEXIST;
>  	struct vport_ops *o;
> @@ -87,7 +87,7 @@ errout:
>  	ovs_unlock();
>  	return err;
>  }
> -EXPORT_SYMBOL_GPL(ovs_vport_ops_register);
> +EXPORT_SYMBOL_GPL(__ovs_vport_ops_register);
>  
>  void ovs_vport_ops_unregister(struct vport_ops *ops)
>  {
> diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
> index bdfd82a..8ea3a96 100644
> --- a/net/openvswitch/vport.h
> +++ b/net/openvswitch/vport.h
> @@ -196,7 +196,13 @@ static inline const char *ovs_vport_name(struct
> vport *vport)
>  	return vport->dev->name;
>  }
>  
> -int ovs_vport_ops_register(struct vport_ops *ops);
> +int __ovs_vport_ops_register(struct vport_ops *ops);
> +#define ovs_vport_ops_register(ops)            \
> +       ({                                      \
> +               (ops)->owner = THIS_MODULE;     \
> +               __ovs_vport_ops_register(ops);  \
> +       })
> +
>  void ovs_vport_ops_unregister(struct vport_ops *ops);

This doesn't look correct (the fault is not your patch).

The register function adds the list_head of the ops struct on a list
without increasing the module reference counter. Thus the module could
be removed and the list walk would walk across the unloaded data section
left over by the module. Does a kmemdup make sense here and a later free
in the unregister function? We test type first and then only touch
functions after getting module refcnt, so it would appear safe to me
then.

Thanks,
Hannes

Re: [PATCH net v2] openvswitch: properly refcount vport-vxlan module

[Paolo Abeni]

On Tue, 2015-12-01 at 13:03 +0100, Hannes Frederic Sowa wrote:
> Hello Paolo,
> 
> On Mon, Nov 30, 2015, at 12:31, Paolo Abeni wrote:
> > After 614732eaa12d, no refcount is maintained for the vport-vxlan module.
> > This allows the userspace to remove such module while vport-vxlan
> > devices still exist, which leads to later oops.
> > 
> > v1 -> v2:
> >  - move vport 'owner' initialization in ovs_vport_ops_register()
> >    and make such function a macro
> > 
> > Fixes: 614732eaa12d ("openvswitch: Use regular VXLAN net_device device")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  net/openvswitch/vport-geneve.c | 1 -
> >  net/openvswitch/vport-gre.c    | 1 -
> >  net/openvswitch/vport.c        | 4 ++--
> >  net/openvswitch/vport.h        | 8 +++++++-
> >  4 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/openvswitch/vport-geneve.c
> > b/net/openvswitch/vport-geneve.c
> > index efb736b..e41cd12 100644
> > --- a/net/openvswitch/vport-geneve.c
> > +++ b/net/openvswitch/vport-geneve.c
> > @@ -117,7 +117,6 @@ static struct vport_ops ovs_geneve_vport_ops = {
> >  	.destroy	= ovs_netdev_tunnel_destroy,
> >  	.get_options	= geneve_get_options,
> >  	.send		= dev_queue_xmit,
> > -       .owner          = THIS_MODULE,
> >  };
> >  
> >  static int __init ovs_geneve_tnl_init(void)
> > diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> > index c3257d7..7f8897f 100644
> > --- a/net/openvswitch/vport-gre.c
> > +++ b/net/openvswitch/vport-gre.c
> > @@ -89,7 +89,6 @@ static struct vport_ops ovs_gre_vport_ops = {
> >  	.create		= gre_create,
> >  	.send		= dev_queue_xmit,
> >  	.destroy	= ovs_netdev_tunnel_destroy,
> > -       .owner          = THIS_MODULE,
> >  };
> >  
> >  static int __init ovs_gre_tnl_init(void)
> > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> > index e194c10a..31cbc8c 100644
> > --- a/net/openvswitch/vport.c
> > +++ b/net/openvswitch/vport.c
> > @@ -71,7 +71,7 @@ static struct hlist_head *hash_bucket(const struct net
> > *net, const char *name)
> >  	return &dev_table[hash & (VPORT_HASH_BUCKETS - 1)];
> >  }
> >  
> > -int ovs_vport_ops_register(struct vport_ops *ops)
> > +int __ovs_vport_ops_register(struct vport_ops *ops)
> >  {
> >  	int err = -EEXIST;
> >  	struct vport_ops *o;
> > @@ -87,7 +87,7 @@ errout:
> >  	ovs_unlock();
> >  	return err;
> >  }
> > -EXPORT_SYMBOL_GPL(ovs_vport_ops_register);
> > +EXPORT_SYMBOL_GPL(__ovs_vport_ops_register);
> >  
> >  void ovs_vport_ops_unregister(struct vport_ops *ops)
> >  {
> > diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
> > index bdfd82a..8ea3a96 100644
> > --- a/net/openvswitch/vport.h
> > +++ b/net/openvswitch/vport.h
> > @@ -196,7 +196,13 @@ static inline const char *ovs_vport_name(struct
> > vport *vport)
> >  	return vport->dev->name;
> >  }
> >  
> > -int ovs_vport_ops_register(struct vport_ops *ops);
> > +int __ovs_vport_ops_register(struct vport_ops *ops);
> > +#define ovs_vport_ops_register(ops)            \
> > +       ({                                      \
> > +               (ops)->owner = THIS_MODULE;     \
> > +               __ovs_vport_ops_register(ops);  \
> > +       })
> > +
> >  void ovs_vport_ops_unregister(struct vport_ops *ops);
> 
> This doesn't look correct (the fault is not your patch).
> 
> The register function adds the list_head of the ops struct on a list
> without increasing the module reference counter. Thus the module could
> be removed and the list walk would walk across the unloaded data section
> left over by the module. Does a kmemdup make sense here and a later free
> in the unregister function? We test type first and then only touch
> functions after getting module refcnt, so it would appear safe to me
> then.

Currently all the vport_ops list manipulation functions are protected by
the ovs_lock (even the lookup): the scenario described above should not
be possible.

Paolo

Re: [PATCH net v2] openvswitch: properly refcount vport-vxlan module

[Hannes Frederic Sowa]

On Tue, Dec 1, 2015, at 14:19, Paolo Abeni wrote:
> Currently all the vport_ops list manipulation functions are protected by
> the ovs_lock (even the lookup): the scenario described above should not
> be possible.

This makes all sense and it seems no use-after-free is possible at all
(I didn't notice the lookup path takes the lock). But the macro still
looks ugly and modifies a global struct with a per-module linked symbol.
David, why exactly didn't you like the first patch? Seems to me it does
what is actually needed.

Thanks,
Hannes

Re: [PATCH net v2] openvswitch: properly refcount vport-vxlan module

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Tue, 01 Dec 2015 16:41:48 +0100

> On Tue, Dec 1, 2015, at 14:19, Paolo Abeni wrote:
>> Currently all the vport_ops list manipulation functions are protected by
>> the ovs_lock (even the lookup): the scenario described above should not
>> be possible.
> 
> This makes all sense and it seems no use-after-free is possible at all
> (I didn't notice the lookup path takes the lock). But the macro still
> looks ugly and modifies a global struct with a per-module linked symbol.
> David, why exactly didn't you like the first patch? Seems to me it does
> what is actually needed.

Because it is error prone.

I want it hidden quietly and reliably in a macro, just like we do for
other similar cases across the tree.

The caller of the registry functions has no reason to be concerned
with these details if we can take care of it transparently.

Re: [PATCH net v2] openvswitch: properly refcount vport-vxlan module

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Mon, 30 Nov 2015 12:31:43 +0100

> After 614732eaa12d, no refcount is maintained for the vport-vxlan module.
> This allows the userspace to remove such module while vport-vxlan
> devices still exist, which leads to later oops.
> 
> v1 -> v2:
>  - move vport 'owner' initialization in ovs_vport_ops_register()
>    and make such function a macro
> 
> Fixes: 614732eaa12d ("openvswitch: Use regular VXLAN net_device device")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied, thanks.