| 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
| ||
|
Message-ID: <20200720102156.717e3e68@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date: Mon, 20 Jul 2020 10:21:56 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Edward Cree <ecree@...arflare.com>
Cc: <davem@...emloft.net>, <netdev@...r.kernel.org>,
<linux-net-drivers@...arflare.com>, <mhabets@...arflare.com>,
<mslattery@...arflare.com>
Subject: Re: [PATCH net-next] efx: convert to new udp_tunnel infrastructure
On Mon, 20 Jul 2020 12:45:54 +0100 Edward Cree wrote:
> Subject line prefix should probably be sfc:, that's what we
> usually use for this driver.
Ack.
> On 18/07/2020 00:53, Jakub Kicinski wrote:
> > Check MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_TRUSTED, before setting
> > the info, which will hopefully protect us from -EPERM errors
> Sorry, I forgot to pass on the answer I got from the firmware
> team, which was "TRUSTED is the wrong thing and there isn't a
> right thing". The TRUSTED flag will be set on any function
> that can set UDP ports, but may also be set on some that can't
> (it's not clear what they're Trusted to do but this isn't it).
> So it's OK to check it, but the EPERMs could still happen.
I'll amend the commit message, hopefully it's good enough in practice,
and perhaps better bit could be added going forward? :(
> > -static int efx_ef10_udp_tnl_add_port(struct efx_nic *efx,
> > - struct efx_udp_tunnel tnl)
> > -{
> > - struct efx_ef10_nic_data *nic_data = efx->nic_data;
> > - struct efx_udp_tunnel *match;
> > - char typebuf[8];
> > - size_t i;
> > - int rc;
> > + if (ti->type == UDP_TUNNEL_TYPE_VXLAN)
> > + efx_tunnel_type = TUNNEL_ENCAP_UDP_PORT_ENTRY_VXLAN;
> > + else
> > + efx_tunnel_type = TUNNEL_ENCAP_UDP_PORT_ENTRY_GENEVE;
> I think I'd prefer to keep the switch() that explicitlychecks
> for UDP_TUNNEL_TYPE_GENEVE; even though the infrastructure
> makes sure it won't ever not be, I'd still feel more comfortable
> that way. But it's up to you.
To me the motivation of expressing capabilities is for the core
to be able to do the necessary checking (and make more intelligent
decisions). All the drivers I've converted make the assumption they
won't see tunnel types they don't support.
> > @@ -3873,6 +3835,7 @@ static int efx_ef10_udp_tnl_add_port(struct efx_nic *efx,
> > static bool efx_ef10_udp_tnl_has_port(struct efx_nic *efx, __be16 port)
> > {
> > struct efx_ef10_nic_data *nic_data = efx->nic_data;
> > + size_t i;
> >
> > if (!(nic_data->datapath_caps &
> > (1 << MC_CMD_GET_CAPABILITIES_OUT_VXLAN_NVGRE_LBN)))
> > @@ -3884,58 +3847,48 @@ static bool efx_ef10_udp_tnl_has_port(struct efx_nic *efx, __be16 port)
> > */
> > return false;
> >
> > - return __efx_ef10_udp_tnl_lookup_port(efx, port) != NULL;
> > + for (i = 0; i < ARRAY_SIZE(nic_data->udp_tunnels); ++i)
> > + if (nic_data->udp_tunnels[i].port == port)
> > + return true;
> > +
> > + return false;
> > }
> >
> > -static int efx_ef10_udp_tnl_del_port(struct efx_nic *efx,
> > - struct efx_udp_tunnel tnl)
> > +static int efx_ef10_udp_tnl_unset_port(struct net_device *dev,
> > + unsigned int table, unsigned int entry,
> > + struct udp_tunnel_info *ti)
> > {
> > - struct efx_ef10_nic_data *nic_data = efx->nic_data;
> > - struct efx_udp_tunnel *match;
> > - char typebuf[8];
> > + struct efx_nic *efx = netdev_priv(dev);
> > + struct efx_ef10_nic_data *nic_data;
> > int rc;
> >
> > - if (!(nic_data->datapath_caps &
> > - (1 << MC_CMD_GET_CAPABILITIES_OUT_VXLAN_NVGRE_LBN)))
> > - return 0;
> > -
> > - efx_get_udp_tunnel_type_name(tnl.type, typebuf, sizeof(typebuf));
> > - netif_dbg(efx, drv, efx->net_dev, "Removing UDP tunnel (%s) port %d\n",
> > - typebuf, ntohs(tnl.port));
> > + nic_data = efx->nic_data;
> >
> > mutex_lock(&nic_data->udp_tunnels_lock);
> > /* Make sure all TX are stopped while we remove from the table, else we
> > * might race against an efx_features_check().
> > */
> > efx_device_detach_sync(efx);
> > -
> > - match = __efx_ef10_udp_tnl_lookup_port(efx, tnl.port);
> > - if (match != NULL) {
> > - if (match->type == tnl.type) {
> > - if (--match->count) {
> > - /* Port is still in use, so nothing to do */
> > - netif_dbg(efx, drv, efx->net_dev,
> > - "UDP tunnel port %d remains active\n",
> > - ntohs(tnl.port));
> > - rc = 0;
> > - goto out_unlock;
> > - }
> > - rc = efx_ef10_set_udp_tnl_ports(efx, false);
> > - goto out_unlock;
> > - }
> > - efx_get_udp_tunnel_type_name(match->type,
> > - typebuf, sizeof(typebuf));
> > - netif_warn(efx, drv, efx->net_dev,
> > - "UDP port %d is actually in use by %s, not removing\n",
> > - ntohs(tnl.port), typebuf);
> > - }
> > - rc = -ENOENT;
> > -
> > -out_unlock:
> > + nic_data->udp_tunnels[entry].port = 0;
> I'm a little concerned that efx_ef10_udp_tnl_has_port(efx, 0);
> willgenerally return true, so in our yet-to-come TX offloads
> patch we'll need to check for !port when handling an skb with
> skb->encapsulation == true before calling has_port.
> (I realise that the kernel almost certainly won't ever give us
> an skb with encap on UDP port 0, but it never hurts to be
> paranoid about things like that).
> Could we not keep a 'valid'/'used' flag in the table, used in
> roughly the same way we were checking count != 0?
How about we do the !port check in efx_ef10_udp_tnl_has_port()?
Per-entry valid / used flag seems a little wasteful.
Another option is to have a reserved tunnel type for invalid / unused.
I don't mind either way.
> Apart from that this all looks fine, and the amount of deleted
> codemakes me happy :)
>
> -ed
Powered by blists - more mailing lists