netdev - Re: [patch net-next 02/11] mlxsw: core_linecards: Introduce per line card auxiliary device

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]

Date:   Thu, 16 Jun 2022 20:41:52 +0300
From:   Ido Schimmel <idosch@...dia.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        petrm@...dia.com, pabeni@...hat.com, edumazet@...gle.com,
        mlxsw@...dia.com
Subject: Re: [patch net-next 02/11] mlxsw: core_linecards: Introduce per line
 card auxiliary device

On Thu, Jun 16, 2022 at 06:39:59PM +0200, Jiri Pirko wrote:
> Thu, Jun 16, 2022 at 09:11:56AM CEST, idosch@...dia.com wrote:
> >On Wed, Jun 15, 2022 at 07:37:15PM +0200, Jiri Pirko wrote:
> >> Wed, Jun 15, 2022 at 04:52:13PM CEST, idosch@...dia.com wrote:
> >> >> +int mlxsw_linecard_bdev_add(struct mlxsw_linecard *linecard)
> >> >> +{
> >> >> +	struct mlxsw_linecard_bdev *linecard_bdev;
> >> >> +	int err;
> >> >> +	int id;
> >> >> +
> >> >> +	id = mlxsw_linecard_bdev_id_alloc();
> >> >> +	if (id < 0)
> >> >> +		return id;
> >> >> +
> >> >> +	linecard_bdev = kzalloc(sizeof(*linecard_bdev), GFP_KERNEL);
> >> >> +	if (!linecard_bdev) {
> >> >> +		mlxsw_linecard_bdev_id_free(id);
> >> >> +		return -ENOMEM;
> >> >> +	}
> >> >> +	linecard_bdev->adev.id = id;
> >> >> +	linecard_bdev->adev.name = MLXSW_LINECARD_DEV_ID_NAME;
> >> >> +	linecard_bdev->adev.dev.release = mlxsw_linecard_bdev_release;
> >> >> +	linecard_bdev->adev.dev.parent = linecard->linecards->bus_info->dev;
> >> >> +	linecard_bdev->linecard = linecard;
> >> >> +
> >> >> +	err = auxiliary_device_init(&linecard_bdev->adev);
> >> >> +	if (err) {
> >> >> +		mlxsw_linecard_bdev_id_free(id);
> >> >> +		kfree(linecard_bdev);
> >> >> +		return err;
> >> >> +	}
> >> >> +
> >> >> +	err = auxiliary_device_add(&linecard_bdev->adev);
> >> >> +	if (err) {
> >> >> +		auxiliary_device_uninit(&linecard_bdev->adev);
> >> >> +		return err;
> >> >> +	}
> >> >> +
> >> >> +	linecard->bdev = linecard_bdev;
> >> >> +	return 0;
> >> >> +}
> >> >
> >> >[...]
> >> >
> >> >> +static int mlxsw_linecard_bdev_probe(struct auxiliary_device *adev,
> >> >> +				     const struct auxiliary_device_id *id)
> >> >> +{
> >> >> +	struct mlxsw_linecard_bdev *linecard_bdev =
> >> >> +			container_of(adev, struct mlxsw_linecard_bdev, adev);
> >> >> +	struct mlxsw_linecard_dev *linecard_dev;
> >> >> +	struct devlink *devlink;
> >> >> +
> >> >> +	devlink = devlink_alloc(&mlxsw_linecard_dev_devlink_ops,
> >> >> +				sizeof(*linecard_dev), &adev->dev);
> >> >> +	if (!devlink)
> >> >> +		return -ENOMEM;
> >> >> +	linecard_dev = devlink_priv(devlink);
> >> >> +	linecard_dev->linecard = linecard_bdev->linecard;
> >> >> +	linecard_bdev->linecard_dev = linecard_dev;
> >> >> +
> >> >> +	devlink_register(devlink);
> >> >> +	return 0;
> >> >> +}
> >> >
> >> >[...]
> >> >
> >> >> @@ -252,6 +253,14 @@ mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
> >> >>  	linecard->provisioned = true;
> >> >>  	linecard->hw_revision = hw_revision;
> >> >>  	linecard->ini_version = ini_version;
> >> >> +
> >> >> +	err = mlxsw_linecard_bdev_add(linecard);
> >> >
> >> >If a line card is already provisioned and we are reloading the primary
> >> >devlink instance, isn't this going to deadlock on the global (not
> >> >per-instance) devlink mutex? It is held throughout the reload operation
> >> >and also taken in devlink_register()
> >> >
> >> >My understanding of the auxiliary bus model is that after adding a
> >> >device to the bus via auxiliary_device_add(), the probe() function of
> >> >the auxiliary driver will be called. In our case, this function acquires
> >> >the global devlink mutex in devlink_register().
> >> 
> >> No, the line card auxdev is supposed to be removed during
> >> linecard_fini(). This, I forgot to add, will do in v2.
> >
> >mlxsw_linecard_fini() is called as part of reload with the global
> >devlink mutex held. The removal of the auxdev should prompt the
> >unregistration of its devlink instance which also takes this mutex. If
> >this doesn't deadlock, then I'm probably missing something.
> 
> You don't miss anything, it really does. Need to remove devlink_mutex
> first.

Can you please send it separately? Will probably need thorough review
and testing...

The comment above devlink_mutex is: "An overall lock guarding every
operation coming from userspace. It also guards devlink devices list and
it is taken when driver registers/unregisters it.", but devlink does not
have "parallel_ops" enabled, so maybe it's enough to only use this lock
to protect the devlink devices list?

> 
> 
> >
> >Can you test reload with lockdep when line cards are already
> >provisioned/active?
> >
> >> 
> >> 
> >> >
> >> >> +	if (err) {
> >> >> +		linecard->provisioned = false;
> >> >> +		mlxsw_linecard_provision_fail(linecard);
> >> >> +		return err;
> >> >> +	}
> >> >> +
> >> >>  	devlink_linecard_provision_set(linecard->devlink_linecard, type);
> >> >>  	return 0;
> >> >>  }