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:
 <SN6PR02MB41570A66CB3660C803BB6063D4562@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Fri, 1 Nov 2024 21:11:00 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Dexuan Cui <decui@...rosoft.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: Dexuan Cui <decui@...rosoft.com> Sent: Friday, November 1, 2024 1:27 PM
> 
> > 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.

I can do it. You probably have more pressing issues to keep
you busy .... :-)

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

Fair enough. I'll do it that way.

Michael