[PATCH] Fix neighbor discovery ethernet address saving

[PATCH] Fix neighbor discovery ethernet address saving

[seanedmond]

From: Sean Edmond <seanedmond@microsoft.com>

When a successful neighbor advertisement is received, the ethernet
address should be saved for later use to avoid having to redo the
neighbor discovery process.

For example, with TFTP the address should get saved into
"net_server_ethaddr".  This is being done correctly with ARP for IPv4,
but not for neighbor discovery with IPv6.

Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
---
 net/ndisc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ndisc.c b/net/ndisc.c
index d1cec0601c8..505515f2d95 100644
--- a/net/ndisc.c
+++ b/net/ndisc.c
@@ -461,8 +461,8 @@ int ndisc_receive(struct ethernet_hdr *et, struct ip6_hdr *ip6, int len)
 			ndisc_extract_enetaddr(ndisc, neigh_eth_addr);
 
 			/* save address for later use */
-			if (!net_nd_packet_mac)
-				net_nd_packet_mac = neigh_eth_addr;
+			if (net_nd_packet_mac)
+				memcpy(net_nd_packet_mac, neigh_eth_addr, 6);
 
 			/* modify header, and transmit it */
 			memcpy(((struct ethernet_hdr *)net_nd_tx_packet)->et_dest,
-- 
2.42.0

Re: [PATCH] Fix neighbor discovery ethernet address saving

[Vyacheslav V. Mitrofanov]

On Mon, 2024-04-29 at 11:51 -0700, seanedmond@linux.microsoft.com
wrote:
> soc@yadro.com<mailto:soc@yadro.com>
> 
> From: Sean Edmond <seanedmond@microsoft.com>
> 
> When a successful neighbor advertisement is received, the ethernet
> address should be saved for later use to avoid having to redo the
> neighbor discovery process.
> 
> For example, with TFTP the address should get saved into
> "net_server_ethaddr".  This is being done correctly with ARP for
> IPv4,
> but not for neighbor discovery with IPv6.
> 
> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> ---
>  net/ndisc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ndisc.c b/net/ndisc.c
> index d1cec0601c8..505515f2d95 100644
> --- a/net/ndisc.c
> +++ b/net/ndisc.c
> @@ -461,8 +461,8 @@ int ndisc_receive(struct ethernet_hdr *et, struct
> ip6_hdr *ip6, int len)
>                         ndisc_extract_enetaddr(ndisc,
> neigh_eth_addr);
> 
>                         /* save address for later use */
> -                       if (!net_nd_packet_mac)
> -                               net_nd_packet_mac = neigh_eth_addr;
> +                       if (net_nd_packet_mac)
> +                               memcpy(net_nd_packet_mac,
> neigh_eth_addr, 6);
> 
>                         /* modify header, and transmit it */
>                         memcpy(((struct ethernet_hdr
> *)net_nd_tx_packet)->et_dest,
> --
> 2.42.0
> 
Hello, Sean. Thanks for your notice. I see that net_nd_packet_mac is
just a uchar pointer without memory allocation. It is dangerous to do 
memcpy and not necessary. All works as it has to be.

Re: [PATCH] Fix neighbor discovery ethernet address saving

[Sean Edmond]

Hi Vyachesla,

Let's start by saying neighbour discovery protocol is definitely working 
properly.  I'm only suggesting that it isn't properly caching the MAC 
address.  During IPv6 TFTP I observe that neighbour discovery is 
performed before every packet sent from client(u-boot)->server.

Here's a snapshot of what the packets from client(u-boot)<->server look 
like during IPv6 TFTP:
-> neighbour solicitation
<- neighbour advertisement
-> read request
<- Block 0
-> neighbour solicitation
<- neighbour advertisement
-> ACK block 0
<- Block 1
-> neighbour solicitation
<- neighbour advertisement
-> ACK block 1
- ... (continues for entire file transfer)

The neighbour discovery on every TX TFTP packet isn't required and 
results in degraded performance.  Note, when performing the test with 
IPv4, ARP is not performed before every TX packet.

Here's a description of the code flow (including my proposal):
- net.c defines "u8 net_server_ethaddr[6];"
- tftp_send()-> net_send_udp_packet6(net_server_ethaddr, ...)
- in net_send_udp_packet6(), "net_nd_packet_mac = ether;"  (now, 
net_nd_packet_mac is pointing to net_server_ethaddr)
- in ndisc_receive(), when NA is received the mac becomes available in 
neigh_eth_addr
- My proposal is, "if pointer net_nd_packet_mac isn't NULL, copy this 
contents of neigh_eth_addr into net_nd_packet_mac"
- For TFTP, my fix allows the server's MAC to get copied into 
net_server_ethaddr
- on the next tftp_send(), in net_send_udp_packet6() neighbour discovery 
is skipped because "ether" isn't all zeros

The memcpy isn't dangerous, because it's copying the discovered mac 
address into the already allocated net_server_ethaddr (and it's checking 
to make sure that net_nd_packet_mac isn't NULL before copying).

Also, the current code serves no purpose.  The current code is, "if 
net_nd_packet_mac is NULL, set it to stack variable neigh_eth_addr, then 
set net_nd_packet_mac to NULL when there is no ND pending (the mac 
address doesn't get saved in net_server_ethaddr).

Sean










On 2024-05-05 2:40 a.m., Vyacheslav V. Mitrofanov wrote:
> On Mon, 2024-04-29 at 11:51 -0700, seanedmond@linux.microsoft.com
> wrote:
>> soc@yadro.com<mailto:soc@yadro.com>
>>
>> From: Sean Edmond <seanedmond@microsoft.com>
>>
>> When a successful neighbor advertisement is received, the ethernet
>> address should be saved for later use to avoid having to redo the
>> neighbor discovery process.
>>
>> For example, with TFTP the address should get saved into
>> "net_server_ethaddr".  This is being done correctly with ARP for
>> IPv4,
>> but not for neighbor discovery with IPv6.
>>
>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>> ---
>>   net/ndisc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ndisc.c b/net/ndisc.c
>> index d1cec0601c8..505515f2d95 100644
>> --- a/net/ndisc.c
>> +++ b/net/ndisc.c
>> @@ -461,8 +461,8 @@ int ndisc_receive(struct ethernet_hdr *et, struct
>> ip6_hdr *ip6, int len)
>>                          ndisc_extract_enetaddr(ndisc,
>> neigh_eth_addr);
>>
>>                          /* save address for later use */
>> -                       if (!net_nd_packet_mac)
>> -                               net_nd_packet_mac = neigh_eth_addr;
>> +                       if (net_nd_packet_mac)
>> +                               memcpy(net_nd_packet_mac,
>> neigh_eth_addr, 6);
>>
>>                          /* modify header, and transmit it */
>>                          memcpy(((struct ethernet_hdr
>> *)net_nd_tx_packet)->et_dest,
>> --
>> 2.42.0
>>
> Hello, Sean. Thanks for your notice. I see that net_nd_packet_mac is
> just a uchar pointer without memory allocation. It is dangerous to do
> memcpy and not necessary. All works as it has to be.

Re: [PATCH] Fix neighbor discovery ethernet address saving

[Vyacheslav V. Mitrofanov]

Hello, Sean. 
I see. You are right.

Reviewed-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com>


On Tue, 2024-05-07 at 22:16 -0700, Sean Edmond wrote:
> 
> Hi Vyachesla,
> 
> Let's start by saying neighbour discovery protocol is definitely
> working
> properly.  I'm only suggesting that it isn't properly caching the MAC
> address.  During IPv6 TFTP I observe that neighbour discovery is
> performed before every packet sent from client(u-boot)->server.
> 
> Here's a snapshot of what the packets from client(u-boot)<->server
> look
> like during IPv6 TFTP:
> -> neighbour solicitation
> <- neighbour advertisement
> -> read request
> <- Block 0
> -> neighbour solicitation
> <- neighbour advertisement
> -> ACK block 0
> <- Block 1
> -> neighbour solicitation
> <- neighbour advertisement
> -> ACK block 1
> - ... (continues for entire file transfer)
> 
> The neighbour discovery on every TX TFTP packet isn't required and
> results in degraded performance.  Note, when performing the test with
> IPv4, ARP is not performed before every TX packet.
> 
> Here's a description of the code flow (including my proposal):
> - net.c defines "u8 net_server_ethaddr[6];"
> - tftp_send()-> net_send_udp_packet6(net_server_ethaddr, ...)
> - in net_send_udp_packet6(), "net_nd_packet_mac = ether;"  (now,
> net_nd_packet_mac is pointing to net_server_ethaddr)
> - in ndisc_receive(), when NA is received the mac becomes available
> in
> neigh_eth_addr
> - My proposal is, "if pointer net_nd_packet_mac isn't NULL, copy this
> contents of neigh_eth_addr into net_nd_packet_mac"
> - For TFTP, my fix allows the server's MAC to get copied into
> net_server_ethaddr
> - on the next tftp_send(), in net_send_udp_packet6() neighbour
> discovery
> is skipped because "ether" isn't all zeros
> 
> The memcpy isn't dangerous, because it's copying the discovered mac
> address into the already allocated net_server_ethaddr (and it's
> checking
> to make sure that net_nd_packet_mac isn't NULL before copying).
> 
> Also, the current code serves no purpose.  The current code is, "if
> net_nd_packet_mac is NULL, set it to stack variable neigh_eth_addr,
> then
> set net_nd_packet_mac to NULL when there is no ND pending (the mac
> address doesn't get saved in net_server_ethaddr).
> 
> Sean
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> On 2024-05-05 2:40 a.m., Vyacheslav V. Mitrofanov wrote:
> > On Mon, 2024-04-29 at 11:51 -0700, seanedmond@linux.microsoft.com
> > wrote:
> > > soc@yadro.com<mailto:soc@yadro.com>
> > > 
> > > From: Sean Edmond <seanedmond@microsoft.com>
> > > 
> > > When a successful neighbor advertisement is received, the
> > > ethernet
> > > address should be saved for later use to avoid having to redo the
> > > neighbor discovery process.
> > > 
> > > For example, with TFTP the address should get saved into
> > > "net_server_ethaddr".  This is being done correctly with ARP for
> > > IPv4,
> > > but not for neighbor discovery with IPv6.
> > > 
> > > Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
> > > ---
> > >   net/ndisc.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/ndisc.c b/net/ndisc.c
> > > index d1cec0601c8..505515f2d95 100644
> > > --- a/net/ndisc.c
> > > +++ b/net/ndisc.c
> > > @@ -461,8 +461,8 @@ int ndisc_receive(struct ethernet_hdr *et,
> > > struct
> > > ip6_hdr *ip6, int len)
> > >                          ndisc_extract_enetaddr(ndisc,
> > > neigh_eth_addr);
> > > 
> > >                          /* save address for later use */
> > > -                       if (!net_nd_packet_mac)
> > > -                               net_nd_packet_mac =
> > > neigh_eth_addr;
> > > +                       if (net_nd_packet_mac)
> > > +                               memcpy(net_nd_packet_mac,
> > > neigh_eth_addr, 6);
> > > 
> > >                          /* modify header, and transmit it */
> > >                          memcpy(((struct ethernet_hdr
> > > *)net_nd_tx_packet)->et_dest,
> > > --
> > > 2.42.0
> > > 
> > Hello, Sean. Thanks for your notice. I see that net_nd_packet_mac
> > is
> > just a uchar pointer without memory allocation. It is dangerous to
> > do
> > memcpy and not necessary. All works as it has to be.

Re: [PATCH] Fix neighbor discovery ethernet address saving

[Tom Rini]

On Mon, 29 Apr 2024 11:51:16 -0700, seanedmond@linux.microsoft.com wrote:

> When a successful neighbor advertisement is received, the ethernet
> address should be saved for later use to avoid having to redo the
> neighbor discovery process.
> 
> For example, with TFTP the address should get saved into
> "net_server_ethaddr".  This is being done correctly with ARP for IPv4,
> but not for neighbor discovery with IPv6.
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom