| Message ID | 20201016045715.26768-1-ljp@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ibmvnic: save changed mac address to adapter->mac_addr | expand |
On Thu, 15 Oct 2020 23:57:15 -0500 Lijun Pan wrote: > After mac address change request completes successfully, the new mac > address need to be saved to adapter->mac_addr as well as > netdev->dev_addr. Otherwise, adapter->mac_addr still holds old > data. Do you observe this in practice? Can you show us which path in particular you see this happen on? AFAICS ibmvnic_set_mac() copies the new MAC addr into adapter->mac_addr before making a request. If anything is wrong here is that it does so regardless if MAC addr is valid. > Fixes: 62740e97881c("net/ibmvnic: Update MAC address settings after adapter reset") ^ missing space here > Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
> On Oct 19, 2020, at 7:11 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 15 Oct 2020 23:57:15 -0500 Lijun Pan wrote: >> After mac address change request completes successfully, the new mac >> address need to be saved to adapter->mac_addr as well as >> netdev->dev_addr. Otherwise, adapter->mac_addr still holds old >> data. > > Do you observe this in practice? Can you show us which path in > particular you see this happen on? > > AFAICS ibmvnic_set_mac() copies the new MAC addr into adapter->mac_addr > before making a request. > > If anything is wrong here is that it does so regardless if MAC addr > is valid. > Yes, I ran some internal test to check the mac address in adapter->mac_addr, and it is the old data. If you run ifconfig command to change mac addr, the netdev->dev_addr is changed afterwards, and if you run ifocnfig again, it will show the new mac addr. However, since we did not check adapter->mac_addr in this use case, this bug was not exposed. This vnic driver is little bit different than other physical NIC driver. All the control paths are negotaited with VIOS server, and data paths are through DMA mapping. __ibmvnic_set_mac copies the new mac addr to crq by ether_addr_copy(&crq.change_mac_addr.mac_addr[0], dev_addr); and then send the change request by rc = ibmvnic_send_crq(adapter, &crq); Now adapter->mac_addr still has the old data. When the request is handled by VIOS server, an interrupt is triggered, and handle_change_mac_rsp is called. Now it is time to copy the new mac to netdev->dev_addr, and adatper->mac_addr. ether_addr_copy(netdev->dev_addr, &crq->change_mac_addr_rsp.mac_addr[0]); It missed the copy for adapter->mac_addr, which is what I add in this patch. + ether_addr_copy(adapter->mac_addr, + &crq->change_mac_addr_rsp.mac_addr[0]); >> Fixes: 62740e97881c("net/ibmvnic: Update MAC address settings after adapter reset") > ^ > missing space here > Will fix this in v2. Lijun
On Tue, 20 Oct 2020 16:18:04 -0500 Lijun Pan wrote: > > On Oct 19, 2020, at 7:11 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 15 Oct 2020 23:57:15 -0500 Lijun Pan wrote: > >> After mac address change request completes successfully, the new mac > >> address need to be saved to adapter->mac_addr as well as > >> netdev->dev_addr. Otherwise, adapter->mac_addr still holds old > >> data. > > > > Do you observe this in practice? Can you show us which path in > > particular you see this happen on? > > > > AFAICS ibmvnic_set_mac() copies the new MAC addr into adapter->mac_addr > > before making a request. > > > > If anything is wrong here is that it does so regardless if MAC addr > > is valid. > > Yes, I ran some internal test to check the mac address in adapter->mac_addr, and > it is the old data. If you run ifconfig command to change mac addr, the netdev->dev_addr > is changed afterwards, and if you run ifocnfig again, it will show the new mac addr. However, > since we did not check adapter->mac_addr in this use case, this bug was not exposed. > > This vnic driver is little bit different than other physical NIC driver. All the control paths > are negotaited with VIOS server, and data paths are through DMA mapping. > > __ibmvnic_set_mac copies the new mac addr to crq by > ether_addr_copy(&crq.change_mac_addr.mac_addr[0], dev_addr); > and then send the change request by > rc = ibmvnic_send_crq(adapter, &crq); > Now adapter->mac_addr still has the old data. > > When the request is handled by VIOS server, an interrupt is triggered, and > handle_change_mac_rsp is called. > Now it is time to copy the new mac to netdev->dev_addr, and adatper->mac_addr. > ether_addr_copy(netdev->dev_addr, > &crq->change_mac_addr_rsp.mac_addr[0]); > It missed the copy for adapter->mac_addr, which is what I add in this patch. > + ether_addr_copy(adapter->mac_addr, > + &crq->change_mac_addr_rsp.mac_addr[0]); Please read my reply carefully. What's the call path that leads to the address being wrong? If you set the address via ifconfig it will call ibmvnic_set_mac() of the driver. ibmvnic_set_mac() does the copy. But it doesn't validate the address, which it should.
> On Oct 20, 2020, at 4:33 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 20 Oct 2020 16:18:04 -0500 Lijun Pan wrote: >>> On Oct 19, 2020, at 7:11 PM, Jakub Kicinski <kuba@kernel.org> wrote: >>> >>> On Thu, 15 Oct 2020 23:57:15 -0500 Lijun Pan wrote: >>>> After mac address change request completes successfully, the new mac >>>> address need to be saved to adapter->mac_addr as well as >>>> netdev->dev_addr. Otherwise, adapter->mac_addr still holds old >>>> data. >>> >>> Do you observe this in practice? Can you show us which path in >>> particular you see this happen on? >>> >>> AFAICS ibmvnic_set_mac() copies the new MAC addr into adapter->mac_addr >>> before making a request. >>> >>> If anything is wrong here is that it does so regardless if MAC addr >>> is valid. >> >> Yes, I ran some internal test to check the mac address in adapter->mac_addr, and >> it is the old data. If you run ifconfig command to change mac addr, the netdev->dev_addr >> is changed afterwards, and if you run ifocnfig again, it will show the new mac addr. However, >> since we did not check adapter->mac_addr in this use case, this bug was not exposed. >> >> This vnic driver is little bit different than other physical NIC driver. All the control paths >> are negotaited with VIOS server, and data paths are through DMA mapping. >> >> __ibmvnic_set_mac copies the new mac addr to crq by >> ether_addr_copy(&crq.change_mac_addr.mac_addr[0], dev_addr); >> and then send the change request by >> rc = ibmvnic_send_crq(adapter, &crq); >> Now adapter->mac_addr still has the old data. >> >> When the request is handled by VIOS server, an interrupt is triggered, and >> handle_change_mac_rsp is called. >> Now it is time to copy the new mac to netdev->dev_addr, and adatper->mac_addr. >> ether_addr_copy(netdev->dev_addr, >> &crq->change_mac_addr_rsp.mac_addr[0]); >> It missed the copy for adapter->mac_addr, which is what I add in this patch. >> + ether_addr_copy(adapter->mac_addr, >> + &crq->change_mac_addr_rsp.mac_addr[0]); > > Please read my reply carefully. > > What's the call path that leads to the address being wrong? If you set > the address via ifconfig it will call ibmvnic_set_mac() of the driver. > ibmvnic_set_mac() does the copy. > > But it doesn't validate the address, which it should. Sorry about that. The mac addr validation is performed on vios side when it receives the change request. That’s why there is no mac validation code in vnic driver. In handle_change_mac_rsp(), &crq->change_mac_addr_rsp.mac_addr[0] contains the current valid mac address, which may be different than the requested one, that is &crq->change_mac_addr.mac_addr[0]. crq->change_mac_addr.mac_addr is the requested one. crq->change_mac_addr_rsp.mac_addr is the returned valid one. Hope the above answers your doubt.
> On Oct 20, 2020, at 5:07 PM, Lijun Pan <ljp@linux.vnet.ibm.com> wrote: > > > >> On Oct 20, 2020, at 4:33 PM, Jakub Kicinski <kuba@kernel.org> wrote: >> >> On Tue, 20 Oct 2020 16:18:04 -0500 Lijun Pan wrote: >>>> On Oct 19, 2020, at 7:11 PM, Jakub Kicinski <kuba@kernel.org> wrote: >>>> >>>> On Thu, 15 Oct 2020 23:57:15 -0500 Lijun Pan wrote: >>>>> After mac address change request completes successfully, the new mac >>>>> address need to be saved to adapter->mac_addr as well as >>>>> netdev->dev_addr. Otherwise, adapter->mac_addr still holds old >>>>> data. >>>> >>>> Do you observe this in practice? Can you show us which path in >>>> particular you see this happen on? >>>> >>>> AFAICS ibmvnic_set_mac() copies the new MAC addr into adapter->mac_addr >>>> before making a request. >>>> >>>> If anything is wrong here is that it does so regardless if MAC addr >>>> is valid. >>> >>> Yes, I ran some internal test to check the mac address in adapter->mac_addr, and >>> it is the old data. If you run ifconfig command to change mac addr, the netdev->dev_addr >>> is changed afterwards, and if you run ifocnfig again, it will show the new mac addr. However, >>> since we did not check adapter->mac_addr in this use case, this bug was not exposed. >>> >>> This vnic driver is little bit different than other physical NIC driver. All the control paths >>> are negotaited with VIOS server, and data paths are through DMA mapping. >>> >>> __ibmvnic_set_mac copies the new mac addr to crq by >>> ether_addr_copy(&crq.change_mac_addr.mac_addr[0], dev_addr); >>> and then send the change request by >>> rc = ibmvnic_send_crq(adapter, &crq); >>> Now adapter->mac_addr still has the old data. >>> >>> When the request is handled by VIOS server, an interrupt is triggered, and >>> handle_change_mac_rsp is called. >>> Now it is time to copy the new mac to netdev->dev_addr, and adatper->mac_addr. >>> ether_addr_copy(netdev->dev_addr, >>> &crq->change_mac_addr_rsp.mac_addr[0]); >>> It missed the copy for adapter->mac_addr, which is what I add in this patch. >>> + ether_addr_copy(adapter->mac_addr, >>> + &crq->change_mac_addr_rsp.mac_addr[0]); >> >> Please read my reply carefully. >> >> What's the call path that leads to the address being wrong? If you set >> the address via ifconfig it will call ibmvnic_set_mac() of the driver. >> ibmvnic_set_mac() does the copy. >> >> But it doesn't validate the address, which it should. > > Sorry about that. The mac addr validation is performed on vios side when it receives the > change request. That’s why there is no mac validation code in vnic driver. > In handle_change_mac_rsp(), &crq->change_mac_addr_rsp.mac_addr[0] > contains the current valid mac address, which may be different than the requested one, > that is &crq->change_mac_addr.mac_addr[0]. > crq->change_mac_addr.mac_addr is the requested one. > crq->change_mac_addr_rsp.mac_addr is the returned valid one. > > Hope the above answers your doubt. > And the crq can be altered by both vnic driver and VIOS server since in init_crq_queue(), it has bidirectional mapping. crq->msg_token = dma_map_single(dev, crq->msgs, PAGE_SIZE, DMA_BIDIRECTIONAL);
On Tue, 20 Oct 2020 17:07:38 -0500 Lijun Pan wrote: > > Please read my reply carefully. > > > > What's the call path that leads to the address being wrong? If you set > > the address via ifconfig it will call ibmvnic_set_mac() of the driver. > > ibmvnic_set_mac() does the copy. > > > > But it doesn't validate the address, which it should. > > Sorry about that. The mac addr validation is performed on vios side when it receives the > change request. That’s why there is no mac validation code in vnic driver. The problem is that there is validation in the driver: static int __ibmvnic_set_mac(struct net_device *netdev, u8 *dev_addr) { struct ibmvnic_adapter *adapter = netdev_priv(netdev); union ibmvnic_crq crq; int rc; if (!is_valid_ether_addr(dev_addr)) { rc = -EADDRNOTAVAIL; goto err; } And ibmvnic_set_mac() does this: ether_addr_copy(adapter->mac_addr, addr->sa_data); if (adapter->state != VNIC_PROBED) rc = __ibmvnic_set_mac(netdev, addr->sa_data); So if state == VNIC_PROBED, the user can assign an invalid address to adapter->mac_addr, and ibmvnic_set_mac() will still return 0. That's a separate issue, for a separate patch, though, so you can send a v2 of this patch regardless. > In handle_change_mac_rsp(), &crq->change_mac_addr_rsp.mac_addr[0] > contains the current valid mac address, which may be different than the requested one, > that is &crq->change_mac_addr.mac_addr[0]. > crq->change_mac_addr.mac_addr is the requested one. > crq->change_mac_addr_rsp.mac_addr is the returned valid one. > > Hope the above answers your doubt. Oh! The address in reply can be different than the requested one? Please add a comment stating that in the code.
> On Oct 20, 2020, at 5:19 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 20 Oct 2020 17:07:38 -0500 Lijun Pan wrote: >>> Please read my reply carefully. >>> >>> What's the call path that leads to the address being wrong? If you set >>> the address via ifconfig it will call ibmvnic_set_mac() of the driver. >>> ibmvnic_set_mac() does the copy. >>> >>> But it doesn't validate the address, which it should. >> >> Sorry about that. The mac addr validation is performed on vios side when it receives the >> change request. That’s why there is no mac validation code in vnic driver. > > The problem is that there is validation in the driver: > > static int __ibmvnic_set_mac(struct net_device *netdev, u8 *dev_addr) > { > struct ibmvnic_adapter *adapter = netdev_priv(netdev); > union ibmvnic_crq crq; > int rc; > > if (!is_valid_ether_addr(dev_addr)) { > rc = -EADDRNOTAVAIL; > goto err; > } > I think this one validates whether the address is of a legal format. The validation in VIOS checks it according to the Address Control List entries set up by the administrator. > And ibmvnic_set_mac() does this: > > ether_addr_copy(adapter->mac_addr, addr->sa_data); > if (adapter->state != VNIC_PROBED) > rc = __ibmvnic_set_mac(netdev, addr->sa_data); > > So if state == VNIC_PROBED, the user can assign an invalid address to > adapter->mac_addr, and ibmvnic_set_mac() will still return 0. Let me address this problem, and send a separate patch. > > That's a separate issue, for a separate patch, though, so you can send > a v2 of this patch regardless. > >> In handle_change_mac_rsp(), &crq->change_mac_addr_rsp.mac_addr[0] >> contains the current valid mac address, which may be different than the requested one, >> that is &crq->change_mac_addr.mac_addr[0]. >> crq->change_mac_addr.mac_addr is the requested one. >> crq->change_mac_addr_rsp.mac_addr is the returned valid one. >> >> Hope the above answers your doubt. > > Oh! The address in reply can be different than the requested one? > Please add a comment stating that in the code. I have sent v2 with comment in the code.
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 1b702a43a5d0..021968d1db9c 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -4196,6 +4196,8 @@ static int handle_change_mac_rsp(union ibmvnic_crq *crq, } ether_addr_copy(netdev->dev_addr, &crq->change_mac_addr_rsp.mac_addr[0]); + ether_addr_copy(adapter->mac_addr, + &crq->change_mac_addr_rsp.mac_addr[0]); out: complete(&adapter->fw_done); return rc;
After mac address change request completes successfully, the new mac address need to be saved to adapter->mac_addr as well as netdev->dev_addr. Otherwise, adapter->mac_addr still holds old data. Fixes: 62740e97881c("net/ibmvnic: Update MAC address settings after adapter reset") Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 2 ++ 1 file changed, 2 insertions(+)