netdev - Re: [PATCH net-next 5/5] devlink: hold the instance lock during eswitch

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: <20220317084110.76b3ba2c@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 17 Mar 2022 08:41:10 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, jiri@...nulli.us,
        saeedm@...dia.com, idosch@...sch.org, michael.chan@...adcom.com,
        simon.horman@...igine.com
Subject: Re: [PATCH net-next 5/5] devlink: hold the instance lock during
 eswitch_mode callbacks

On Thu, 17 Mar 2022 09:52:10 +0200 Leon Romanovsky wrote:
> On Wed, Mar 16, 2022 at 09:20:23PM -0700, Jakub Kicinski wrote:
> > Make the devlink core hold the instance lock during eswitch_mode
> > callbacks. Cheat in case of mlx5 (see the cover letter).  
> 
> And this is one the main difference between your and mine proposals/solutions.
> I didn't want to cheat as it doesn't help to the end goal - remove devlink_mutex.
> 
> Can you please change the comments in mlx5 to be more direct?
> "/* TODO: convert the driver to devl_* */"
> ->  
> "/* FIXME: devl_unlock() followed by devl_lock() inside driver callback
>   * never correct and prone to races. Never repeat this pattern.
>   *
>   * This code MUST be fixed before removing devlink_mutex as it is safe
>   * to do only because of that mutex.
>   */"
> 
> Something like that.

Should I add this comment in all the spots I'm adding the re-locking?
Does it make sense to add a wrapper:

/* FIXME: devl_unlock() followed by devl_lock() inside driver callback
 * never correct and prone to races. Never repeat this pattern.
 *
 * This code MUST be fixed before removing devlink_mutex as it is safe
 * to do only because of that mutex.
 */
static void mlx5_eswtich_mode_callback_enter(...)
{
	devl_unlock(devlink);
 	down_write(&esw->mode_lock);
}

and respective helper for "leave" ?

> The code is correct, but like I said before, I don't like the direction.
> I expect that this anti-pattern is going to be copy/pasted very soon.

Yeah, this is most certainly a very temporary hack.

BTW is there a chance for me to access a fully featured mlx5 system
somehow to test the future conversion patches? Or perhaps you or
someone on the team would be interested in doing the conversion?

> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@...dia.com>

Thanks!