On 4/22/19 2:54 PM, Jakub Kicinski wrote: > On Mon, 22 Apr 2019 10:27:54 -0700, Tao Ren wrote: >> Previously BMC's MAC address is calculated by simply adding 1 to the >> last byte of network controller's MAC address, and it produces incorrect >> result when network controller's MAC address ends with 0xFF. >> The problem is fixed by detecting integer overflow when incrementing MAC >> address and adding the carry bit (if any) to the next/left bytes of the >> MAC address. >> > > It'd be good to have a Fixes tag, if it's worth going to the net tree.
Thank you for the quick review Jakub. Sure, I will update the patch description with Fixes tag accordingly. >> Signed-off-by: Tao Ren <tao...@fb.com> >> --- >> net/ncsi/ncsi-rsp.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c >> index dc07fcc7938e..eb42bbdb7501 100644 >> --- a/net/ncsi/ncsi-rsp.c >> +++ b/net/ncsi/ncsi-rsp.c >> @@ -658,7 +658,8 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct >> ncsi_request *nr) >> const struct net_device_ops *ops = ndev->netdev_ops; >> struct ncsi_rsp_oem_pkt *rsp; >> struct sockaddr saddr; >> - int ret = 0; >> + int ret, offset; >> + u16 carry = 1; >> >> /* Get the response header */ >> rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp); >> @@ -667,7 +668,12 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct >> ncsi_request *nr) >> ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE; >> memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN); >> /* Increase mac address by 1 for BMC's address */ >> - saddr.sa_data[ETH_ALEN - 1]++; >> + offset = ETH_ALEN - 1; >> + do { >> + carry += (u8)saddr.sa_data[offset]; >> + saddr.sa_data[offset] = (char)carry; >> + carry = carry >> 8; >> + } while (carry != 0 && --offset >= 0); > > We have eth_addr_dec(), perhaps it'd be good to add an eth_addr_inc() > equivalent? (I'm not sure if it'd have to be in net-next, it's a tiny > function, and OK for net for my taste, but I had been wrong before). Make sense. I will split the patch to 2 then: 1) add eth_addr_inc() into linux/etherdevice.h 2) fixes overflow when incrementing mac address by calling eth_addr_inc() function. > If I'm allowed to be paranoid I'd also advise checking the resulting > MAC is a valid ethernet unicast addr. > >> ret = ops->ndo_set_mac_address(ndev, &saddr); >> if (ret < 0) >> netdev_warn(ndev, "NCSI: 'Writing mac address to device >> failed\n"); Thanks for the suggestion. Will add the check. - Tao