Date: Tue, 19 Mar 2024 15:52:08 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>, Karthik Sundaravel
	<ksundara@...hat.com>, Suman Ghosh <sumang@...vell.com>
CC: "pmenzel@...gen.mpg.de" <pmenzel@...gen.mpg.de>, "aharivel@...hat.com"
	<aharivel@...hat.com>, "michal.swiatkowski@...ux.intel.com"
	<michal.swiatkowski@...ux.intel.com>, "jiri@...nulli.us" <jiri@...nulli.us>,
	"cfontain@...hat.com" <cfontain@...hat.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "anthony.l.nguyen@...el.com"
	<anthony.l.nguyen@...el.com>, "vchundur@...hat.com" <vchundur@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "kuba@...nel.org"
	<kuba@...nel.org>, "rjarry@...hat.com" <rjarry@...hat.com>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "davem@...emloft.net"
	<davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [EXTERNAL] [PATCH v5] ice: Add get/set hw
 address for VFs using devlink commands



On 3/18/2024 8:04 AM, Wojciech Drewek wrote:
> 
> 
> On 18.03.2024 12:55, Karthik Sundaravel wrote:
>> On Fri, Mar 8, 2024 at 3:28 PM Suman Ghosh <sumang@...vell.com> wrote:
>>>
>>>> ----------------------------------------------------------------------
>>>> Changing the MAC address of the VFs are not available via devlink. Add
>>>> the function handlers to set and get the HW address for the VFs.
>>>>
>>>> Signed-off-by: Karthik Sundaravel <ksundara@...hat.com>
>>>> ---
>>>> drivers/net/ethernet/intel/ice/ice_devlink.c | 78 +++++++++++++++++++-
>>>> drivers/net/ethernet/intel/ice/ice_sriov.c   | 62 ++++++++++++++++
>>>> drivers/net/ethernet/intel/ice/ice_sriov.h   |  8 ++
>>>> 3 files changed, 147 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c
>>>> b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>>> index 80dc5445b50d..39d4d79ac731 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>>> @@ -1576,6 +1576,81 @@ void ice_devlink_destroy_pf_port(struct ice_pf
>>>> *pf)
>>>>       devlink_port_unregister(&pf->devlink_port);
>>>> }
>>>>
>>>> +/**
>>>> + * ice_devlink_port_get_vf_fn_mac - .port_fn_hw_addr_get devlink
>>>> +handler
>>>> + * @port: devlink port structure
>>>> + * @hw_addr: MAC address of the port
>>>> + * @hw_addr_len: length of MAC address
>>>> + * @extack: extended netdev ack structure
>>>> + *
>>>> + * Callback for the devlink .port_fn_hw_addr_get operation
>>>> + * Return: zero on success or an error code on failure.
>>>> + */
>>>> +
>>>> +static int ice_devlink_port_get_vf_fn_mac(struct devlink_port *port,
>>>> +                                        u8 *hw_addr, int *hw_addr_len,
>>>> +                                        struct netlink_ext_ack *extack)
>>>> +{
>>>> +      struct devlink_port_attrs *attrs = &port->attrs;
>>> [Suman] I agree with Wojciech about using container_of:
>>
>> [Karthik] when I use container_of(), on some occasions I get core dump
>> in get and set functions.
>> These issues were not seen in the earlier versions.
>> Can you please suggest any pointers on what could have gone wrong ?
>>
>> struct ice_vf *vf = container_of(port, struct ice_vf, devlink_port);
>>
>> [  597.658325] ------------[ cut here ]------------
>> [  597.658329] refcount_t: underflow; use-after-free.
>> [  597.658430] CPU: 18 PID: 1926 Comm: devlink Not tainted 6.8.0-rc5-dirty #1
>> [  ...]
>> [  597.658506]  ? refcount_warn_saturate+0xbe/0x110
>> [  597.658509]  ice_devlink_port_get_vf_fn_mac+0x39/0x70 [ice]
>> [  597.658607]  ? __pfx_ice_devlink_port_get_vf_fn_mac+0x10/0x10 [ice]
>> [  597.658676]  devlink_nl_port_fill+0x314/0xa30
>> [  ...]
>> [  597.658835] ---[ end trace 0000000000000000 ]---
>>
>>
>> [  859.989482] ------------[ cut here ]------------
>> [  859.989485] refcount_t: saturated; leaking memory.
>> [  859.989500] WARNING: CPU: 0 PID: 1965 at lib/refcount.c:19
>> refcount_warn_saturate+0x9b/0x110
>> [  ...]
>> [  859.989671]  ? refcount_warn_saturate+0x9b/0x110
>> [  859.989674]  ice_get_vf_by_id+0x87/0xa0 [ice]
>> [  859.989777]  ice_set_vf_fn_mac+0x33/0x150 [ice]
>> [  859.989858]  ice_devlink_port_set_vf_fn_mac+0x61/0x90 [ice]
>> [  859.989940]  devlink_nl_port_set_doit+0x1d3/0x610
>> [  ...]
>> [  952.413933] ---[ end trace 0000000000000000 ]---
> 
> Ok, I think we forgot about kref here.
> Once you have a VF pointer you have to inc the ref count using
> kref_get_unless_zero and you have to check return value because the VF
> might be already freed. When you don't need the VF's pointer anymore call ice_put_vf.
> Would be cool to have Jake's opinion on that though since he implemented it.
> 

If you can get a VF from a devlink_port which is embedded in the ice_vf
structure, then you must guarantee that port is still valid for as long
as the VF is. This means that you can't delete the last reference of the
VF until the port is removed I think. This is tricky because we have
multiple ways to get to the VF now.

What manages the life cycle of the devlink port associated with the VF?

If you obtain the pointer to the VF via "container_of" on a valid
devlink port, you have to assume that having the port implies having a
reference to the VF, and that the port won't be destroyed before the VF.
For this reason ,you would a) not use get_vf_by_id, and also b) not
dereference the refcount using ice_put_vf.

I suspect that you accidentally still called ice_put_vf() in the case
where you used container of?

Powered by blists - more mailing lists