[PATCH] net: avoid assigning ethaddr to wrong devices

[PATCH] net: avoid assigning ethaddr to wrong devices

[Nikita Yushchenko]

It can happen that device tree contains ethernetN alias pointing to
valid device, but that device is not supported by [running instance of]
barebox. Then ethN remains unassigned, and can be later captured by
dynamically registered device such as usbnet.

For such "stranger" device, ethaddr preconfigured for ethN should not be
assigned. Also, ethaddr of such device should not be written to
ethernetN node of device tree passed to kernel being booted.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 net/eth.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 5d45a0461..4ec0d3a30 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -60,6 +60,38 @@ int eth_set_ethaddr(struct eth_device *edev, const char *ethaddr)
 	return 0;
 }
 
+#ifdef CONFIG_OFTREE
+static bool eth_is_stranger(struct eth_device *edev)
+{
+	/*
+	 * It is possible that device tree has ethernetN alias pointing to
+	 * a node representing device, but that device is not supported by
+	 * [this instance of] barebox, thus ethN remains unassigned. Then
+	 * dynamic device such as usbnet can be registered and become ethN.
+	 *
+	 * For such "stranger" device, preset ethaddr for ethN should not
+	 * be applied. Also ethaddr of such device should not be written
+	 * to ethernetN node of device tree passed to booted kernel
+	 */
+	char eth[12];
+	struct device_node *alias_node;
+
+	sprintf(eth, "ethernet%d", edev->dev.id);
+	alias_node = of_find_node_by_alias(of_get_root_node(), eth);
+
+	if (alias_node) {
+		/* alias exists, any other device with that id is "stranger" */
+		return !(edev->parent &&
+			 edev->parent->device_node == alias_node);
+	} else {
+		/* alias does not exist, thus not "stranger" */
+		return false;
+	}
+}
+#else
+#define eth_is_stranger(edev) (0)
+#endif
+
 static void register_preset_mac_address(struct eth_device *edev, const char *ethaddr)
 {
 	unsigned char ethaddr_str[sizeof("xx:xx:xx:xx:xx:xx")];
@@ -81,7 +113,7 @@ static int eth_get_registered_ethaddr(struct eth_device *edev, void *buf)
 
 	list_for_each_entry(addr, &ethaddr_list, list) {
 		if ((node && node == addr->node) ||
-				addr->ethid == edev->dev.id) {
+		    (addr->ethid == edev->dev.id && !eth_is_stranger(edev))) {
 			memcpy(buf, addr->ethaddr, 6);
 			return 0;
 		}
@@ -286,7 +318,7 @@ static void eth_of_fixup_node(struct device_node *root,
 			      const char *node_path, int ethid,
 			      const u8 ethaddr[6])
 {
-	struct device_node *node;
+	struct device_node *node = NULL;
 	int ret;
 
 	if (!is_valid_ether_addr(ethaddr)) {
@@ -297,7 +329,7 @@ static void eth_of_fixup_node(struct device_node *root,
 
 	if (node_path) {
 		node = of_find_node_by_path_from(root, node_path);
-	} else {
+	} else if (ethid >= 0) {
 		char eth[12];
 		sprintf(eth, "ethernet%d", ethid);
 		node = of_find_node_by_alias(root, eth);
@@ -331,7 +363,9 @@ static int eth_of_fixup(struct device_node *root, void *unused)
 				  addr->ethid, addr->ethaddr);
 
 	for_each_netdev(edev)
-		eth_of_fixup_node(root, edev->nodepath, edev->dev.id, edev->ethaddr);
+		eth_of_fixup_node(root, edev->nodepath,
+				  eth_is_stranger(edev) ? -1 : edev->dev.id,
+				  edev->ethaddr);
 
 	return 0;
 }
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Sascha Hauer]

Hi Nikita,

On Fri, Jun 22, 2018 at 07:30:12PM +0300, Nikita Yushchenko wrote:
> It can happen that device tree contains ethernetN alias pointing to
> valid device, but that device is not supported by [running instance of]
> barebox. Then ethN remains unassigned, and can be later captured by
> dynamically registered device such as usbnet.
> 
> For such "stranger" device, ethaddr preconfigured for ethN should not be
> assigned. Also, ethaddr of such device should not be written to
> ethernetN node of device tree passed to kernel being booted.
> 

There's only one usecase for matching edev->dev.id against the ethernetx
alias which has been introduced with:

| commit a78431c7fc42193be252417bf06f7cc61765a51e
| Author: Renaud Barbier <renaud.barbier@ge.com>
| Date:   Wed Sep 4 08:37:03 2013 +0200
| 
|     net, of: fixup MAC address by alias
|     
|     If a network device has not been registered from the devicetree, we may
|     still find it by its alias in the devicetree. This way also platform based
|     network devices can obtain a valid MAC address in the devicetree.
|     
|     Signed-off-by: Renaud Barbier <renaud.barbier@ge.com>
|     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Your eth_is_stranger() returns true for the devices that Renaud wanted
to support, so instead of applying your patch we could equally well
revert that from Renaud.

I don't have a good idea right now how to fix this. Maybe we have to
make sure that ethernet devices from dynamic buses never get an id
asigned that is also present in the aliases node.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Nikita Yushchenko]

25.06.2018 15:34, Sascha Hauer wrote:
> Hi Nikita,
> 
> On Fri, Jun 22, 2018 at 07:30:12PM +0300, Nikita Yushchenko wrote:
>> It can happen that device tree contains ethernetN alias pointing to
>> valid device, but that device is not supported by [running instance of]
>> barebox. Then ethN remains unassigned, and can be later captured by
>> dynamically registered device such as usbnet.
>>
>> For such "stranger" device, ethaddr preconfigured for ethN should not be
>> assigned. Also, ethaddr of such device should not be written to
>> ethernetN node of device tree passed to kernel being booted.
>>
> 
> There's only one usecase for matching edev->dev.id against the ethernetx
> alias which has been introduced with:
> 
> | commit a78431c7fc42193be252417bf06f7cc61765a51e
> | Author: Renaud Barbier <renaud.barbier@ge.com>
> | Date:   Wed Sep 4 08:37:03 2013 +0200
> | 
> |     net, of: fixup MAC address by alias
> |     
> |     If a network device has not been registered from the devicetree, we may
> |     still find it by its alias in the devicetree. This way also platform based
> |     network devices can obtain a valid MAC address in the devicetree.
> |     
> |     Signed-off-by: Renaud Barbier <renaud.barbier@ge.com>
> |     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Your eth_is_stranger() returns true for the devices that Renaud wanted
> to support, so instead of applying your patch we could equally well
> revert that from Renaud.
> 
> I don't have a good idea right now how to fix this. Maybe we have to
> make sure that ethernet devices from dynamic buses never get an id
> asigned that is also present in the aliases node.

eth_is_stranger() for ethN returns true only if ethernetN alias exists
AND ethN either does not have device tree node, or has node different
from what is pointed by alias.

My assumption was that if under linux ethdevice is configured via device
tree node with ethernetX alias, then under barebox it should also be
configured via device tree node with same alias.

You mean, there is hardware that breaks this assumption?
Which hardware it is?
Maybe support for that hardware should be fixed (i.e. appropriate node
and/or alias added to device tree used for that hardware by barebox)?

Nikita

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Sascha Hauer]

On Tue, Jun 26, 2018 at 08:42:37AM +0300, Nikita Yushchenko wrote:
> 
> 
> 25.06.2018 15:34, Sascha Hauer wrote:
> > Hi Nikita,
> > 
> > On Fri, Jun 22, 2018 at 07:30:12PM +0300, Nikita Yushchenko wrote:
> >> It can happen that device tree contains ethernetN alias pointing to
> >> valid device, but that device is not supported by [running instance of]
> >> barebox. Then ethN remains unassigned, and can be later captured by
> >> dynamically registered device such as usbnet.
> >>
> >> For such "stranger" device, ethaddr preconfigured for ethN should not be
> >> assigned. Also, ethaddr of such device should not be written to
> >> ethernetN node of device tree passed to kernel being booted.
> >>
> > 
> > There's only one usecase for matching edev->dev.id against the ethernetx
> > alias which has been introduced with:
> > 
> > | commit a78431c7fc42193be252417bf06f7cc61765a51e
> > | Author: Renaud Barbier <renaud.barbier@ge.com>
> > | Date:   Wed Sep 4 08:37:03 2013 +0200
> > | 
> > |     net, of: fixup MAC address by alias
> > |     
> > |     If a network device has not been registered from the devicetree, we may
> > |     still find it by its alias in the devicetree. This way also platform based
> > |     network devices can obtain a valid MAC address in the devicetree.
> > |     
> > |     Signed-off-by: Renaud Barbier <renaud.barbier@ge.com>
> > |     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > Your eth_is_stranger() returns true for the devices that Renaud wanted
> > to support, so instead of applying your patch we could equally well
> > revert that from Renaud.
> > 
> > I don't have a good idea right now how to fix this. Maybe we have to
> > make sure that ethernet devices from dynamic buses never get an id
> > asigned that is also present in the aliases node.
> 
> eth_is_stranger() for ethN returns true only if ethernetN alias exists
> AND ethN either does not have device tree node, or has node different
> from what is pointed by alias.
> 
> My assumption was that if under linux ethdevice is configured via device
> tree node with ethernetX alias, then under barebox it should also be
> configured via device tree node with same alias.
> 
> You mean, there is hardware that breaks this assumption?
> Which hardware it is?

It's PowerPC hardware which on barebox is not probed from devicetree, so
indeed there is no device node.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Nikita Yushchenko]

>>>> It can happen that device tree contains ethernetN alias pointing to
>>>> valid device, but that device is not supported by [running instance of]
>>>> barebox. Then ethN remains unassigned, and can be later captured by
>>>> dynamically registered device such as usbnet.
>>>>
>>>> For such "stranger" device, ethaddr preconfigured for ethN should not be
>>>> assigned. Also, ethaddr of such device should not be written to
>>>> ethernetN node of device tree passed to kernel being booted.
>>>>
>>>
>>> There's only one usecase for matching edev->dev.id against the ethernetx
>>> alias which has been introduced with:
>>>
>>> | commit a78431c7fc42193be252417bf06f7cc61765a51e
>>> | Author: Renaud Barbier <renaud.barbier@ge.com>
>>> | Date:   Wed Sep 4 08:37:03 2013 +0200
>>> | 
>>> |     net, of: fixup MAC address by alias
>>> |     
>>> |     If a network device has not been registered from the devicetree, we may
>>> |     still find it by its alias in the devicetree. This way also platform based
>>> |     network devices can obtain a valid MAC address in the devicetree.
>>> |     
>>> |     Signed-off-by: Renaud Barbier <renaud.barbier@ge.com>
>>> |     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>>
>>> Your eth_is_stranger() returns true for the devices that Renaud wanted
>>> to support, so instead of applying your patch we could equally well
>>> revert that from Renaud.
>>>
>>> I don't have a good idea right now how to fix this. Maybe we have to
>>> make sure that ethernet devices from dynamic buses never get an id
>>> asigned that is also present in the aliases node.
>>
>> eth_is_stranger() for ethN returns true only if ethernetN alias exists
>> AND ethN either does not have device tree node, or has node different
>> from what is pointed by alias.
>>
>> My assumption was that if under linux ethdevice is configured via device
>> tree node with ethernetX alias, then under barebox it should also be
>> configured via device tree node with same alias.
>>
>> You mean, there is hardware that breaks this assumption?
>> Which hardware it is?
> 
> It's PowerPC hardware which on barebox is not probed from devicetree, so
> indeed there is no device node.

But if no device tree, then
  alias = of_find_node_by_alias(of_get_root_node(), eth);
should return NULL, and eth_is_stranger() should return false, thus
making my patch no-op?

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Sascha Hauer]

On Tue, Jun 26, 2018 at 09:04:02AM +0300, Nikita Yushchenko wrote:
> >>>> It can happen that device tree contains ethernetN alias pointing to
> >>>> valid device, but that device is not supported by [running instance of]
> >>>> barebox. Then ethN remains unassigned, and can be later captured by
> >>>> dynamically registered device such as usbnet.
> >>>>
> >>>> For such "stranger" device, ethaddr preconfigured for ethN should not be
> >>>> assigned. Also, ethaddr of such device should not be written to
> >>>> ethernetN node of device tree passed to kernel being booted.
> >>>>
> >>>
> >>> There's only one usecase for matching edev->dev.id against the ethernetx
> >>> alias which has been introduced with:
> >>>
> >>> | commit a78431c7fc42193be252417bf06f7cc61765a51e
> >>> | Author: Renaud Barbier <renaud.barbier@ge.com>
> >>> | Date:   Wed Sep 4 08:37:03 2013 +0200
> >>> | 
> >>> |     net, of: fixup MAC address by alias
> >>> |     
> >>> |     If a network device has not been registered from the devicetree, we may
> >>> |     still find it by its alias in the devicetree. This way also platform based
> >>> |     network devices can obtain a valid MAC address in the devicetree.
> >>> |     
> >>> |     Signed-off-by: Renaud Barbier <renaud.barbier@ge.com>
> >>> |     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >>>
> >>> Your eth_is_stranger() returns true for the devices that Renaud wanted
> >>> to support, so instead of applying your patch we could equally well
> >>> revert that from Renaud.
> >>>
> >>> I don't have a good idea right now how to fix this. Maybe we have to
> >>> make sure that ethernet devices from dynamic buses never get an id
> >>> asigned that is also present in the aliases node.
> >>
> >> eth_is_stranger() for ethN returns true only if ethernetN alias exists
> >> AND ethN either does not have device tree node, or has node different
> >> from what is pointed by alias.
> >>
> >> My assumption was that if under linux ethdevice is configured via device
> >> tree node with ethernetX alias, then under barebox it should also be
> >> configured via device tree node with same alias.
> >>
> >> You mean, there is hardware that breaks this assumption?
> >> Which hardware it is?
> > 
> > It's PowerPC hardware which on barebox is not probed from devicetree, so
> > indeed there is no device node.
> 
> But if no device tree, then
>   alias = of_find_node_by_alias(of_get_root_node(), eth);
> should return NULL, and eth_is_stranger() should return false, thus
> making my patch no-op?

Ok, you are right. I misread the code. You call of_find_node_by_alias()
on the internal device tree, not the one the Kernel is started with, so
indeed eth_is_stranger() should return false.

Nevertheless I do not like this patch very much as it adds more code to
a place that is already hard to understand in all of its consequences.

I would like to explore the route that we assign these dynamic devices
an id that is not present in any alias node. That could be done by
searching for the highest alias number and give the dynamic devices one
number higher. Would that be doable?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Nikita Yushchenko]

>>> It's PowerPC hardware which on barebox is not probed from devicetree, so
>>> indeed there is no device node.
>>
>> But if no device tree, then
>>   alias = of_find_node_by_alias(of_get_root_node(), eth);
>> should return NULL, and eth_is_stranger() should return false, thus
>> making my patch no-op?
> 
> Ok, you are right. I misread the code. You call of_find_node_by_alias()
> on the internal device tree, not the one the Kernel is started with, so
> indeed eth_is_stranger() should return false.
> 
> Nevertheless I do not like this patch very much as it adds more code to
> a place that is already hard to understand in all of its consequences.
> 
> I would like to explore the route that we assign these dynamic devices
> an id that is not present in any alias node. That could be done by
> searching for the highest alias number and give the dynamic devices one
> number higher. Would that be doable?

Dynamic device number is assigned via
- setting id to DEVICE_ID_DYNAMIC, either by driver or by code at top of
eth_register(),
- replacing that with lowest currently-unused number at top of
register_device()

Probably we can add one more magic value that driver could set into
edev->dev.id before calling eth_register(), that will be replaced with
lowest currently-unused number that does not have aliases. However, this
will change eth numbering in existing setups and thus can break them.

Possible option could be a flag in edev that forbids setting/exporting
ethaddr for this device. Doing so for usbnet seems safe. This will fix
my case.

Nikita

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Nikita Yushchenko]

26.06.2018 10:05, Nikita Yushchenko wrote:
>>>> It's PowerPC hardware which on barebox is not probed from devicetree, so
>>>> indeed there is no device node.
>>>
>>> But if no device tree, then
>>>   alias = of_find_node_by_alias(of_get_root_node(), eth);
>>> should return NULL, and eth_is_stranger() should return false, thus
>>> making my patch no-op?
>>
>> Ok, you are right. I misread the code. You call of_find_node_by_alias()
>> on the internal device tree, not the one the Kernel is started with, so
>> indeed eth_is_stranger() should return false.
>>
>> Nevertheless I do not like this patch very much as it adds more code to
>> a place that is already hard to understand in all of its consequences.
>>
>> I would like to explore the route that we assign these dynamic devices
>> an id that is not present in any alias node. That could be done by
>> searching for the highest alias number and give the dynamic devices one
>> number higher. Would that be doable?
> 
> Dynamic device number is assigned via
> - setting id to DEVICE_ID_DYNAMIC, either by driver or by code at top of
> eth_register(),
> - replacing that with lowest currently-unused number at top of
> register_device()
> 
> Probably we can add one more magic value that driver could set into
> edev->dev.id before calling eth_register(), that will be replaced with
> lowest currently-unused number that does not have aliases. However, this
> will change eth numbering in existing setups and thus can break them.
> 
> Possible option could be a flag in edev that forbids setting/exporting
> ethaddr for this device. Doing so for usbnet seems safe. This will fix
> my case.

Thinking more on this, maybe cleaner is NOT to match by id of in-barebox
ethdevice when setting/exporting ethaddr by default, but explicitly
allow such matching for platforms that need it.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: avoid assigning ethaddr to wrong devices

[Sascha Hauer]

On Tue, Jun 26, 2018 at 10:09:36AM +0300, Nikita Yushchenko wrote:
> 
> 
> 26.06.2018 10:05, Nikita Yushchenko wrote:
> >>>> It's PowerPC hardware which on barebox is not probed from devicetree, so
> >>>> indeed there is no device node.
> >>>
> >>> But if no device tree, then
> >>>   alias = of_find_node_by_alias(of_get_root_node(), eth);
> >>> should return NULL, and eth_is_stranger() should return false, thus
> >>> making my patch no-op?
> >>
> >> Ok, you are right. I misread the code. You call of_find_node_by_alias()
> >> on the internal device tree, not the one the Kernel is started with, so
> >> indeed eth_is_stranger() should return false.
> >>
> >> Nevertheless I do not like this patch very much as it adds more code to
> >> a place that is already hard to understand in all of its consequences.
> >>
> >> I would like to explore the route that we assign these dynamic devices
> >> an id that is not present in any alias node. That could be done by
> >> searching for the highest alias number and give the dynamic devices one
> >> number higher. Would that be doable?
> > 
> > Dynamic device number is assigned via
> > - setting id to DEVICE_ID_DYNAMIC, either by driver or by code at top of
> > eth_register(),
> > - replacing that with lowest currently-unused number at top of
> > register_device()
> > 
> > Probably we can add one more magic value that driver could set into
> > edev->dev.id before calling eth_register(), that will be replaced with
> > lowest currently-unused number that does not have aliases. However, this
> > will change eth numbering in existing setups and thus can break them.
> > 
> > Possible option could be a flag in edev that forbids setting/exporting
> > ethaddr for this device. Doing so for usbnet seems safe. This will fix
> > my case.
> 
> Thinking more on this, maybe cleaner is NOT to match by id of in-barebox
> ethdevice when setting/exporting ethaddr by default, but explicitly
> allow such matching for platforms that need it.

Yes, that would be a good solution. Maybe struct eth_device could get a
new char *of_alias field which the network driver populates from the
platform_data.

Since Renauds address bounces I'm not sure how much interest he still
has in the barebox port of the boards using this feature. I think the
devices are all registered with fsl_eth_init(). I would think that he
does the board fixup himself if he still has interest, but we should
prepare the network device generic part.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox