Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1564191 > unrolled thread

[PATCH net] net: dsa: Keep a reference count on ethernet_dev

Back to article view | Back to linux.kernel

Contents

  [PATCH net] net: dsa: Keep a reference count on ethernet_dev Florian Fainelli <f.fainelli@gmail.com> - 2017-01-21 18:50 +0100
    Re: [PATCH net] net: dsa: Keep a reference count on ethernet_dev David Miller <davem@davemloft.net> - 2017-01-24 18:40 +0100
      Re: [PATCH net] net: dsa: Keep a reference count on ethernet_dev Florian Fainelli <f.fainelli@gmail.com> - 2017-01-24 19:00 +0100
      Re: [PATCH net] net: dsa: Keep a reference count on ethernet_dev Florian Fainelli <f.fainelli@gmail.com> - 2017-01-24 23:30 +0100

#1564191 — [PATCH net] net: dsa: Keep a reference count on ethernet_dev

of_find_net_device_by_node() just returns a reference to a net_device but does
not increment its reference count, which means that the master network device
can just vanish under our feet.

Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index da3862124545..4adb9b11c22c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -440,6 +440,8 @@ static void dsa_dst_unapply(struct dsa_switch_tree *dst)
 
 	pr_info("DSA: tree %d unapplied\n", dst->tree);
 	dst->applied = false;
+
+	dev_put(dst->master_netdev);
 }
 
 static int dsa_cpu_parse(struct device_node *port, u32 index,
@@ -458,6 +460,8 @@ static int dsa_cpu_parse(struct device_node *port, u32 index,
 	if (!ethernet_dev)
 		return -EPROBE_DEFER;
 
+	dev_hold(ethernet_dev);
+
 	if (!ds->master_netdev)
 		ds->master_netdev = ethernet_dev;
 
@@ -473,6 +477,7 @@ static int dsa_cpu_parse(struct device_node *port, u32 index,
 	dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
 	if (IS_ERR(dst->tag_ops)) {
 		dev_warn(ds->dev, "No tagger for this switch\n");
+		dev_put(ethernet_dev);
 		return PTR_ERR(dst->tag_ops);
 	}
 
-- 
2.9.3

[toc] | [next] | [standalone]

#1566019

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Sat, 21 Jan 2017 09:40:54 -0800

> of_find_net_device_by_node() just returns a reference to a net_device but does
> not increment its reference count, which means that the master network device
> can just vanish under our feet.
> 
> Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

This is fine, except now this netdev is completely locked into place with
no way to dynamically unload it.

If someone tries to modunload the driver for this ethernet device,
their screen will fill up with warning messages indicating that the
reference taken here in the DSA code is not going away.

You need to implement a netdev notifier that tears down this DSA
instance during an unregister event and releases the ethernet_dev.
Similar to how we handle protocol addresses bound to a netdev, etc.

[toc] | [prev] | [next] | [standalone]

#1566028

On 01/24/2017 09:39 AM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Sat, 21 Jan 2017 09:40:54 -0800
> 
>> of_find_net_device_by_node() just returns a reference to a net_device but does
>> not increment its reference count, which means that the master network device
>> can just vanish under our feet.
>>
>> Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> This is fine, except now this netdev is completely locked into place with
> no way to dynamically unload it.
> 
> If someone tries to modunload the driver for this ethernet device,
> their screen will fill up with warning messages indicating that the
> reference taken here in the DSA code is not going away.
> 
> You need to implement a netdev notifier that tears down this DSA
> instance during an unregister event and releases the ethernet_dev.
> Similar to how we handle protocol addresses bound to a netdev, etc.

OK, that was actually my original approach, and then while detaching the
switch from the network device seemed easy enough, re-attaching it could
be a little challenging. Let's try again.

Thanks!
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1566204

On 01/24/2017 09:39 AM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Sat, 21 Jan 2017 09:40:54 -0800
> 
>> of_find_net_device_by_node() just returns a reference to a net_device but does
>> not increment its reference count, which means that the master network device
>> can just vanish under our feet.
>>
>> Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> This is fine, except now this netdev is completely locked into place with
> no way to dynamically unload it.
> 
> If someone tries to modunload the driver for this ethernet device,
> their screen will fill up with warning messages indicating that the
> reference taken here in the DSA code is not going away.
> 
> You need to implement a netdev notifier that tears down this DSA
> instance during an unregister event and releases the ethernet_dev.
> Similar to how we handle protocol addresses bound to a netdev, etc.

I have been thinking about this a bit more, and this is what is going on:

- upon master network device unregister we can look up the
dsa_switch_tree in dev->dsa_ptr and call dsa_dst_unapply() that is
actually exactly what we want since it detaches the dsa_switch_tree from
the master network device

- upon master network device register, we can look up whether this
master netdev is the one of interest and re-attach the dangling switch
tree, but here we have two cases:

	- if we have an OF enabled system, doing a reverse look up of
net_device to device to device_node, and then looking it up in the
Device Tree is possible and reasonably simple, this works

	- if we have a platform data enabled system, doing a reverse lookup is
possible, but won't necessarily yield the expected results, because
platform data will have a device reference to the original master
netdev, and this one could be totally different the second time we probe
the master network device due to to unregister/register

Thoughts?
-- 
Florian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web