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 10:11:56 +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 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.

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;
> >>  }