| Message ID | 20201021060712.48806-1-ljp@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] ibmvnic: fix ibmvnic_set_mac | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 2 this patch: 2 |
| jkicinski/kdoc | success | Errors and warnings before: 18 this patch: 18 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| jkicinski/build_allmodconfig_warn | fail | Errors and warnings before: 2 this patch: 2 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Wed, Oct 21, 2020 at 01:07:12AM -0500, Lijun Pan wrote: > Jakub Kicinski brought up a concern in ibmvnic_set_mac(). > 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. > > The fix is to add the handling for "adapter->state == VNIC_PROBED" case, > which saves the old mac address back to adapter->mac_addr, and > returns an error code. > > Fixes: 62740e97881c ("net/ibmvnic: Update MAC address settings after adapter reset") > Cc: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > --- > v2: change the subject from v1's > "ibmvnic: no need to update adapter->mac_addr before it completes" > handle adapter->state==VNIC_PROBED case in else statement. > > drivers/net/ethernet/ibm/ibmvnic.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > index 4dd3625a4fbc..0d78e1e3d44c 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -1829,8 +1829,12 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p) > > rc = 0; > ether_addr_copy(adapter->mac_addr, addr->sa_data); > - if (adapter->state != VNIC_PROBED) > + if (adapter->state != VNIC_PROBED) { > rc = __ibmvnic_set_mac(netdev, addr->sa_data); > + } else { > + ether_addr_copy(adapter->mac_addr, netdev->dev_addr); > + rc = -EIO; Why suddenly you want to change the behavior for case when ibmvnic_set_mac is called for VNIC_PROBED state? I went through the previous discussion and I have a feeling that Jakub meant to simply call the is_valid_ether_addr() on addr->sa_data before the first ether_addr_copy and then act accordingly based on the validity of user supplied mac addr. And instead of yet another write to adapter->mac_addr that you're introducing you could just move the first ether_addr_copy (if addr->sa_data is valid) onto the if (adapter->state != VNIC_PROBED) condition. Right? > + } > > return rc; > } > -- > 2.23.0 >
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 4dd3625a4fbc..0d78e1e3d44c 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -1829,8 +1829,12 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p) rc = 0; ether_addr_copy(adapter->mac_addr, addr->sa_data); - if (adapter->state != VNIC_PROBED) + if (adapter->state != VNIC_PROBED) { rc = __ibmvnic_set_mac(netdev, addr->sa_data); + } else { + ether_addr_copy(adapter->mac_addr, netdev->dev_addr); + rc = -EIO; + } return rc; }
Jakub Kicinski brought up a concern in ibmvnic_set_mac(). 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. The fix is to add the handling for "adapter->state == VNIC_PROBED" case, which saves the old mac address back to adapter->mac_addr, and returns an error code. Fixes: 62740e97881c ("net/ibmvnic: Update MAC address settings after adapter reset") Cc: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- v2: change the subject from v1's "ibmvnic: no need to update adapter->mac_addr before it completes" handle adapter->state==VNIC_PROBED case in else statement. drivers/net/ethernet/ibm/ibmvnic.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)