linux-kernel - RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

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:
 <SA1PR21MB13170389BCF4A5A05FE359EDBF562@SA1PR21MB1317.namprd21.prod.outlook.com>
Date: Fri, 1 Nov 2024 20:26:36 +0000
From: Dexuan Cui <decui@...rosoft.com>
To: Michael Kelley <mhklinux@...look.com>, KY Srinivasan <kys@...rosoft.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, Vitaly
 Kuznetsov <vkuznets@...hat.com>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, "open list:Hyper-V/Azure CORE AND DRIVERS"
	<linux-hyperv@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
CC: "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not
 initialized yet

> From: Michael Kelley <mhklinux@...look.com>
> Sent: Thursday, October 31, 2024 6:39 PM
> > > From: Michael Kelley <mhklinux@...look.com>
> > > Sent: Wednesday, October 30, 2024 5:12 PM
> > > [...]
> > > What do you think about this (compile tested only), which splits the
> > > "init" function into two parts for devices that have char devs? I'm
> > > trying to avoid adding yet another synchronization point by just
> > > doing the init operations in the right order -- i.e., don't create the
> > > user space /dev entry until the VMBus channel is ready.
> > >
> > > Michael
> >
> > Thanks, I think this works! This is a better fix.

Michael, will you post a formal patch or want me to do it?
Either works for me.

> > > +	if (srv->util_init_transport) {
> > > +		ret = srv->util_init_transport();
> > > +		if (ret) {
> > > +			ret = -ENODEV;
> > IMO we don't need the line above, since the 'ret' from
> > srv->util_init_transport()  is already a standard error code.
> 
> I was just now looking at call_driver_probe(), and it behaves
> slightly differently for ENODEV and ENXIO vs. other error
> codes. ENODEV and ENXIO don't output a message to the
> console unless debugging is enabled, while other error codes
> always output a message to the console. Forcing the error to
> ENODEV has been there since the util_probe() code came out
> of staging in year 2011. But I don't really have a preference
> either way.

util_probe() is called by vmbus_probe(), which uses pr_err() to print
the 'ret'. If the 'ret' is forced to ENODEV, the message in vmbus_probe()
may be a little misleading since the real error code is hidden,
especially when srv->util_init_transport() doesn't print any error
message.

vmbus_probe() is called by call_driver_probe. I guess originally
KY wanted to use ENODEV to avoid the extra message for the util
devices in call_driver_probe() in non-debugging mode, but the other
VSC drivers don't follow this usage.

util_probe() can return a non-ENODEV error code anyway, e.g.
ENOMEM and whatever error code from vmbus_open(). IMO,
srv->util_init and srv->util_init_transport should not be treated
specially.

IMO it's better to not add new code to force the 'ret' to
ENODEV, and we'd want to clean up the existing use of ENODEV
in util_probe().

Thanks,
Dexuan