Re: [PATCH] Fix neighbor discovery ethernet address saving

["Vyacheslav V. Mitrofanov" <v.v.mitrofanov@yadro.com>]

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.