| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 5 Jun 2024 19:44:12 +0100
From: Simon Horman <horms@...nel.org>
To: Mengyuan Lou <mengyuanlou@...-swift.com>
Cc: netdev@...r.kernel.org, jiawenwu@...stnetic.com,
duanqiangwen@...-swift.com
Subject: Re: [PATCH net-next v4 4/6] net: libwx: Add msg task func
On Tue, Jun 04, 2024 at 11:57:33PM +0800, Mengyuan Lou wrote:
> Implement wx_msg_task which is used to process mailbox
> messages sent by vf.
>
> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>
Hi Mengyuan Lou,
Some minor comments from my side.
...
> +static int wx_set_vf_mac_addr(struct wx *wx, u32 *msgbuf, u16 vf)
> +{
> + u8 *new_mac = ((u8 *)(&msgbuf[1]));
> +
> + if (!is_valid_ether_addr(new_mac)) {
> + wx_err(wx, "VF %d attempted to set invalid mac\n", vf);
> + return -EINVAL;
> + }
> +
> + if (wx->vfinfo[vf].pf_set_mac &&
> + memcmp(wx->vfinfo[vf].vf_mac_addr, new_mac, ETH_ALEN)) {
> + wx_err(wx,
> + "VF %d attempted to set a MAC address but it already had a MAC address.",
> + vf);
> + return -EBUSY;
> + }
> + return wx_set_vf_mac(wx, vf, new_mac) < 0;
This seems to return a bool - true on error, false otherwise.
But I think it would be more natural to consistently return
a negative error value on error - as is done above, and 0 on success.
So perhaps something like this (completely untested!):
err = wx_set_vf_mac(wx, vf, index, new_mac);
if (err)
return err;
return 0;
> +}
...
> +static int wx_set_vf_lpe(struct wx *wx, u32 max_frame, u32 vf)
> +{
> + struct net_device *netdev = wx->netdev;
> + u32 index, vf_bit, vfre;
> + u32 max_frs, reg_val;
> + int pf_max_frame;
> + int err = 0;
> +
> + pf_max_frame = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> + switch (wx->vfinfo[vf].vf_api) {
> + case wx_mbox_api_11 ... wx_mbox_api_13:
> + /* Version 1.1 supports jumbo frames on VFs if PF has
> + * jumbo frames enabled which means legacy VFs are
> + * disabled
> + */
> + if (pf_max_frame > ETH_FRAME_LEN)
> + break;
> + fallthrough;
> + default:
> + /* If the PF or VF are running w/ jumbo frames enabled
> + * we need to shut down the VF Rx path as we cannot
> + * support jumbo frames on legacy VFs
> + */
> + if (pf_max_frame > ETH_FRAME_LEN ||
> + (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN + VLAN_HLEN)))
> + err = -EINVAL;
> + break;
> + }
> +
> + /* determine VF receive enable location */
> + vf_bit = vf % 32;
> + index = vf / 32;
> +
> + /* enable or disable receive depending on error */
> + vfre = rd32(wx, WX_RDM_VF_RE(index));
> + if (err)
> + vfre &= ~BIT(vf_bit);
> + else
> + vfre |= BIT(vf_bit);
> + wr32(wx, WX_RDM_VF_RE(index), vfre);
> +
> + if (err) {
> + wx_err(wx, "VF max_frame %d out of range\n", max_frame);
> + return err;
> + }
> + /* pull current max frame size from hardware */
> + max_frs = DIV_ROUND_UP(max_frame, 1024);
> + reg_val = rd32(wx, WX_MAC_WDG_TIMEOUT) & WX_MAC_WDG_TIMEOUT_WTO_MASK;
> + if (max_frs > (reg_val + WX_MAC_WDG_TIMEOUT_WTO_DELTA))
> + wr32(wx, WX_MAC_WDG_TIMEOUT, max_frs - WX_MAC_WDG_TIMEOUT_WTO_DELTA);
nit: This could trivially be line wrapped to <= 80 columns wide
wr32(wx, WX_MAC_WDG_TIMEOUT,
max_frs - WX_MAC_WDG_TIMEOUT_WTO_DELTA);
Flagged by checkpatch.pl --max-line-length=80
> +
> + return 0;
> +}
...
> +static int wx_set_vf_macvlan_msg(struct wx *wx, u32 *msgbuf, u16 vf)
> +{
> + int index = (msgbuf[0] & WX_VT_MSGINFO_MASK) >>
> + WX_VT_MSGINFO_SHIFT;
> + u8 *new_mac = ((u8 *)(&msgbuf[1]));
> + int err;
> +
> + if (wx->vfinfo[vf].pf_set_mac && index > 0) {
> + wx_err(wx, "VF %d requested MACVLAN filter but is administratively denied\n", vf);
> + return -EINVAL;
> + }
> +
> + /* An non-zero index indicates the VF is setting a filter */
> + if (index) {
> + if (!is_valid_ether_addr(new_mac)) {
> + wx_err(wx, "VF %d attempted to set invalid mac\n", vf);
> + return -EINVAL;
> + }
> + /* If the VF is allowed to set MAC filters then turn off
> + * anti-spoofing to avoid false positives.
> + */
> + if (wx->vfinfo[vf].spoofchk_enabled)
> + wx_set_vf_spoofchk(wx->netdev, vf, false);
> + }
> +
> + err = wx_set_vf_macvlan(wx, vf, index, new_mac);
> + if (err == -ENOSPC)
> + wx_err(wx,
> + "VF %d has requested a MACVLAN filter but there is no space for it\n",
> + vf);
> +
> + return err < 0;
As per my comment on wx_set_vf_mac_addr(),
this return scheme seems a little unorthodox.
I'd suggest something like this (completely untested!):
err = wx_set_vf_macvlan(wx, vf, index, new_mac);
if (err == -ENOSPC)
wx_err(...)
if (err)
return err;
return 0;
> +}
...
> +static int wx_update_vf_xcast_mode(struct wx *wx, u32 *msgbuf, u32 vf)
> +{
> + int xcast_mode = msgbuf[1];
> + u32 vmolr, disable, enable;
> +
> + /* verify the PF is supporting the correct APIs */
> + switch (wx->vfinfo[vf].vf_api) {
> + case wx_mbox_api_12:
> + /* promisc introduced in 1.3 version */
> + if (xcast_mode == WXVF_XCAST_MODE_PROMISC)
> + return -EOPNOTSUPP;
> + fallthrough;
> + case wx_mbox_api_13:
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + if (wx->vfinfo[vf].xcast_mode == xcast_mode)
> + goto out;
> +
> + switch (xcast_mode) {
> + case WXVF_XCAST_MODE_NONE:
> + disable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE |
> + WX_PSR_VM_L2CTL_MPE | WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;
nit: This could also be trivially line-wrapped to less than 80 columns wide.
Likewise a few times below.
> + enable = 0;
> + break;
> + case WXVF_XCAST_MODE_MULTI:
> + disable = WX_PSR_VM_L2CTL_MPE | WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;
> + enable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE;
> + break;
> + case WXVF_XCAST_MODE_ALLMULTI:
> + disable = WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;
> + enable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE | WX_PSR_VM_L2CTL_MPE;
> + break;
> + case WXVF_XCAST_MODE_PROMISC:
> + disable = 0;
> + enable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE |
> + WX_PSR_VM_L2CTL_MPE | WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + vmolr = rd32(wx, WX_PSR_VM_L2CTL(vf));
> + vmolr &= ~disable;
> + vmolr |= enable;
> + wr32(wx, WX_PSR_VM_L2CTL(vf), vmolr);
> +
> + wx->vfinfo[vf].xcast_mode = xcast_mode;
> +out:
> + msgbuf[1] = xcast_mode;
> +
> + return 0;
> +}
...
Powered by blists - more mailing lists