netdev - Re: [Intel-wired-lan] [iwl-next v1 04/15] ice: add basic devlink subfunctions support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZctSGPf6v0QlfMUu@nanopsycho>
Date: Tue, 13 Feb 2024 12:27:20 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
Cc: maciej.fijalkowski@...el.com, netdev@...r.kernel.org,
	michal.kubiak@...el.com, intel-wired-lan@...ts.osuosl.org,
	pio.raczynski@...il.com, sridhar.samudrala@...el.com,
	jacob.e.keller@...el.com, wojciech.drewek@...el.com,
	Piotr Raczynski <piotr.raczynski@...el.com>,
	przemyslaw.kitszel@...el.com
Subject: Re: [Intel-wired-lan] [iwl-next v1 04/15] ice: add basic devlink
 subfunctions support

Tue, Feb 13, 2024 at 10:39:47AM CET, michal.swiatkowski@...ux.intel.com wrote:
>On Tue, Feb 13, 2024 at 09:55:20AM +0100, Jiri Pirko wrote:
>> Tue, Feb 13, 2024 at 08:27:13AM CET, michal.swiatkowski@...ux.intel.com wrote:
>> >From: Piotr Raczynski <piotr.raczynski@...el.com>

[...]


>
>> 
>> >+}
>> >+
>> >+/**
>> >+ * ice_dealloc_dynamic_port - Deallocate and remove a dynamic port
>> >+ * @dyn_port: dynamic port instance to deallocate
>> >+ *
>> >+ * Free resources associated with a dynamically added devlink port. Will
>> >+ * deactivate the port if its currently active.
>> >+ */
>> >+static void ice_dealloc_dynamic_port(struct ice_dynamic_port *dyn_port)
>> >+{
>> >+	struct devlink_port *devlink_port = &dyn_port->devlink_port;
>> >+	struct ice_pf *pf = dyn_port->pf;
>> >+
>> >+	if (dyn_port->active)
>> >+		ice_deactivate_dynamic_port(dyn_port);
>> >+
>> >+	if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_SF)
>> 
>> I don't understand how this check could be false. Remove it.
>>
>Yeah, will remove
>
>> 
>> >+		xa_erase(&pf->sf_nums, devlink_port->attrs.pci_sf.sf);
>> >+
>> >+	devl_port_unregister(devlink_port);
>> >+	ice_vsi_free(dyn_port->vsi);
>> >+	xa_erase(&pf->dyn_ports, dyn_port->vsi->idx);
>> >+	kfree(dyn_port);
>> >+}
>> >+
>> >+/**
>> >+ * ice_dealloc_all_dynamic_ports - Deallocate all dynamic devlink ports
>> >+ * @pf: pointer to the pf structure
>> >+ */
>> >+void ice_dealloc_all_dynamic_ports(struct ice_pf *pf)
>> >+{
>> >+	struct devlink *devlink = priv_to_devlink(pf);
>> >+	struct ice_dynamic_port *dyn_port;
>> >+	unsigned long index;
>> >+
>> >+	devl_lock(devlink);
>> >+	xa_for_each(&pf->dyn_ports, index, dyn_port)
>> >+		ice_dealloc_dynamic_port(dyn_port);
>> >+	devl_unlock(devlink);
>> 
>> Hmm, I would assume that the called should already hold the devlink
>> instance lock when doing remove. What is stopping user from issuing
>> port_new command here, after devl_unlock()?
>>
>It is only called from remove path, but I can move it upper.

I know it is called on remove path. Again, what is stopping user from
issuing port_new after ice_dealloc_all_dynamic_ports() is called?

[...]


>> 
>> >+	struct device *dev = ice_pf_to_dev(pf);
>> >+	int err;
>> >+
>> >+	dev_dbg(dev, "%s flavour:%d index:%d pfnum:%d\n", __func__,
>> >+		new_attr->flavour, new_attr->port_index, new_attr->pfnum);
>> 
>> How this message could ever help anyone?
>>
>Probably only developer of the code :p, will remove it

How exactly?

[...]


>> >+static int ice_sf_cfg_netdev(struct ice_dynamic_port *dyn_port)
>> >+{
>> >+	struct net_device *netdev;
>> >+	struct ice_vsi *vsi = dyn_port->vsi;
>> >+	struct ice_netdev_priv *np;
>> >+	int err;
>> >+
>> >+	netdev = alloc_etherdev_mqs(sizeof(*np), vsi->alloc_txq,
>> >+				    vsi->alloc_rxq);
>> >+	if (!netdev)
>> >+		return -ENOMEM;
>> >+
>> >+	SET_NETDEV_DEV(netdev, &vsi->back->pdev->dev);
>> >+	set_bit(ICE_VSI_NETDEV_ALLOCD, vsi->state);
>> >+	vsi->netdev = netdev;
>> >+	np = netdev_priv(netdev);
>> >+	np->vsi = vsi;
>> >+
>> >+	ice_set_netdev_features(netdev);
>> >+
>> >+	netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
>> >+			       NETDEV_XDP_ACT_XSK_ZEROCOPY |
>> >+			       NETDEV_XDP_ACT_RX_SG;
>> >+
>> >+	eth_hw_addr_set(netdev, dyn_port->hw_addr);
>> >+	ether_addr_copy(netdev->perm_addr, dyn_port->hw_addr);
>> >+	netdev->netdev_ops = &ice_sf_netdev_ops;
>> >+	SET_NETDEV_DEVLINK_PORT(netdev, &dyn_port->devlink_port);
>> >+
>> >+	err = register_netdev(netdev);
>> 
>> It the the actual subfunction or eswitch port representor of the
>> subfunction. Looks like the port representor. In that case. It should be
>> created no matter if the subfunction is activated, when it it created.
>> 
>> If this is the actual subfunction netdev, you should not link it to
>> devlink port here.
>>
>This is the actual subfunction netdev. Where in this case it should be
>linked?

To the SF auxdev, obviously.

Here, you should have eswitch port representor netdev.