netdev - Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

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: <Y4JEGYMtIWX9clxo@lunn.ch>
Date:   Sat, 26 Nov 2022 17:51:37 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc:     linux-can@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        netdev@...r.kernel.org, linux-usb@...r.kernel.org,
        Saeed Mahameed <saeed@...nel.org>,
        Jiri Pirko <jiri@...dia.com>,
        Lukas Magel <lukas.magel@...teo.net>
Subject: Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

> @@ -2196,11 +2198,12 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
>  		ops = &es581_4_ops;
>  	}
>  
> -	es58x_dev = devm_kzalloc(dev, es58x_sizeof_es58x_device(param),
> -				 GFP_KERNEL);
> -	if (!es58x_dev)
> +	devlink = devlink_alloc(&es58x_dl_ops, es58x_sizeof_es58x_device(param),
> +				dev);
> +	if (!devlink)
>  		return ERR_PTR(-ENOMEM);
>  
> +	es58x_dev = devlink_priv(devlink);

That is 'interesting'. Makes me wonder about lifetimes of different
objects. Previously your es58x_dev structure would disappear when the
driver is released, or an explicit call to devm_kfree(). Now it
disappears when devlink_free() is called. Any danger of use after free
here?

USB devices always make me wonder about life times rules since they
are probably the mode dynamic sort of device the kernel has the
handle, them just abruptly disappearing.

>  	es58x_dev->param = param;
>  	es58x_dev->ops = ops;
>  	es58x_dev->dev = dev;
> @@ -2247,6 +2250,8 @@ static int es58x_probe(struct usb_interface *intf,
>  	if (ret)
>  		return ret;
>  
> +	devlink_register(priv_to_devlink(es58x_dev));
> +
>  	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
>  		ret = es58x_init_netdev(es58x_dev, ch_idx);
>  		if (ret) {
> @@ -2272,8 +2277,10 @@ static void es58x_disconnect(struct usb_interface *intf)
>  	dev_info(&intf->dev, "Disconnecting %s %s\n",
>  		 es58x_dev->udev->manufacturer, es58x_dev->udev->product);
>  
> +	devlink_unregister(priv_to_devlink(es58x_dev));
>  	es58x_free_netdevs(es58x_dev);
>  	es58x_free_urbs(es58x_dev);
> +	devlink_free(priv_to_devlink(es58x_dev));
>  	usb_set_intfdata(intf, NULL);

Should devlink_free() be after usb_set_inftdata()?

       Andrew