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:   Wed, 15 Jun 2022 17:52:13 +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

> +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().

> +	if (err) {
> +		linecard->provisioned = false;
> +		mlxsw_linecard_provision_fail(linecard);
> +		return err;
> +	}
> +
>  	devlink_linecard_provision_set(linecard->devlink_linecard, type);
>  	return 0;
>  }