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: <dafcfb71-52b5-4bf7-8145-aae2dfc06e10@intel.com>
Date: Tue, 27 Aug 2024 08:58:33 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<linux-kernel@...r.kernel.org>, <kernel-janitors@...r.kernel.org>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, "Pavan Kumar
 Linga" <pavan.kumar.linga@...el.com>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH net-next] idpf: Slightly simplify memory management in
 idpf_add_del_mac_filters()

On 8/26/24 19:14, Christophe JAILLET wrote:
> Le 26/08/2024 à 11:15, Przemek Kitszel a écrit :
>> On 8/23/24 11:10, Dan Carpenter wrote:
>>> On Fri, Aug 23, 2024 at 08:23:29AM +0200, Christophe JAILLET wrote:

>>> It would be even nicer to move the ma_list allocation outside the loop:
>>>
>>>          buf_size = struct_size(ma_list, mac_addr_list, 
>>> IDPF_NUM_FILTERS_PER_MSG);
>>>          ma_list = kmalloc(buf_size, GFP_ATOMIC);
>>
>> good point
>>
>> I've opened whole function for inspection and it asks for even more,
>> as of now, we allocate an array in atomic context, just to have a copy
>> of some stuff from the spinlock-protected list.
>>
>> It would be good to have allocation as pointed by Dan prior to iteration
>> and fill it on the fly, sending when new message would not fit plus once
>> at the end. Sending procedure is safe to be called under a spinlock.
> 
> If I understand correctly, you propose to remove the initial copy in 
> mac_addr and hold &vport_config->mac_filter_list_lock till the end of 
> the function?
> 
> That's it?

You got it right. Thanks for your further analysis below.

> 
> There is a wait_for_completion_timeout() in idpf_vc_xn_exec() and the 
> default time-out is IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC    (60 * 1000)
> 
> So, should an issue occurs, and the time out run till the end, we could 
> hold the 'mac_filter_list_lock' spinlock for up to 60 seconds?
> Is that ok?

Messing with this list while we are not done processing does not sound
right either.
But the most concerning part for me with my proposition is that it could
be very slow to just "abort and unload the driver".

> 
> 
> And if in asynch update mode, idpf_mac_filter_async_handler() also takes 
> &vport_config->mac_filter_list_lock;. Could we dead-lock?

indeed looks so :( sorry

> 
> 
> So, I'm not sure to understand what you propose, or the code in 
> idpf_add_del_mac_filters() and co.
> 
>>
>> CCing author; CCing Olek to ask if there are already some refactors that
>> would conflict with this.
> 
> I'll way a few days for these feedbacks and send a v2.

Would be good to have simple memory cleanup first, and later (if at all)
to untangle our locks a bit.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ