linux-kernel - RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

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] [day] [month] [year] [list]

Date:	Mon, 2 Feb 2015 02:37:48 +0000
From:	Dexuan Cui <decui@...rosoft.com>
To:	KY Srinivasan <kys@...rosoft.com>,
	Vitaly Kuznetsov <vkuznets@...hat.com>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"driverdev-devel@...uxdriverproject.org" 
	<driverdev-devel@...uxdriverproject.org>,
	"olaf@...fle.de" <olaf@...fle.de>,
	"apw@...onical.com" <apw@...onical.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>
Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

> -----Original Message-----
> From: KY Srinivasan
> Sent: Monday, February 2, 2015 3:42 AM
> To: Dexuan Cui; Vitaly Kuznetsov
> Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org; driverdev-
> devel@...uxdriverproject.org; olaf@...fle.de; apw@...onical.com;
> jasowang@...hat.com; Haiyang Zhang
> Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
> > -----Original Message-----
> > From: Dexuan Cui
> > Sent: Thursday, January 29, 2015 9:03 PM
> > To: Vitaly Kuznetsov
> > Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org; driverdev-
> > devel@...uxdriverproject.org; olaf@...fle.de; apw@...onical.com;
> > jasowang@...hat.com; KY Srinivasan; Haiyang Zhang
> > Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > ENOMEM
> >
> > > -----Original Message-----
> > > From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
> > > Sent: Thursday, January 29, 2015 21:22 PM
> > > To: Dexuan Cui
> > > Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org;
> > > driverdev- devel@...uxdriverproject.org; olaf@...fle.de;
> > > apw@...onical.com; jasowang@...hat.com; KY Srinivasan; Haiyang Zhang
> > > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > > ENOMEM
> > >
> > > Dexuan Cui <decui@...rosoft.com> writes:
> > >
> > > > Without this patch, the state is put to CHANNEL_OPENING_STATE, and
> > > > when the driver is loaded next time, vmbus_open() will fail
> > > > immediately due to
> > > > newchannel->state != CHANNEL_OPEN_STATE.
> > >
> > > The patch makes sense, but I have one small doubt. We call vmbus_open
> > > from probe functions of various devices. E.g. in hyperv-keyboard we
> > > have:
> > >  error = vmbus_open(...)
> > >  if (error)
> > >      goto err_free_mem;
> > >
> > > and we don't call vmbus_close(...) on this path so no
> > > CHANNELMSG_CLOSECHANNEL will be send.
> 
> If vmbus_open() fails, depending on where the failure occurred, the necessary
> rollback is expected to have
> been done in the vmbus_open() function itself. In vmbus_open function  the last
> thing we do is to send the request
> to the host to open the channel. If this fails, then there is no need to close the
> channel.
Got it.

> > Exactly.
> >
> > > Who's gonna retry probe?
> > The user can try 'rmmod' and 'modprobe' the module to re-probe.
> >
> > > Wouldn't it be better to close the channel?
> > Good question!
> We close the channel only if the open() of the channel succeeded.
Got it.

> >
> > In your example, due to " goto err_free_mem", we don't run vmbus_close(),
> > so the memory allocated for the ringbuffer is actually leaked!
> >
> > Next time when we reload the module, vmbus_open() will allocate new
> > memory for the ringbuffer.
> 
> I must be missing something here. When vmbus_open() fails, we will rollback the
> state and free up the memory
> allocated for the ring buffer.
KY,
Sorry, I didn't check the code carefully...
You're correct. There is no issue here.

> 
> As I look at the vmbus_open() code I do see an issue with regards cleaning up
> the gpadl state and I am sending a patch for this
> shortly. I will base this on top of the this series from Dexuan.
Thanks, KY!
I'm not sure you meant the below line at the end of vmbus_open():
        if (err == 0)
                newchannel->state = CHANNEL_OPENED_STATE;
        ELSE
	???

-- Dexuan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/