linux-kernel - Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20161206082142.6795f1c3@xeon-e3>
Date:   Tue, 6 Dec 2016 08:21:42 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Long Li <longli@...rosoft.com>
Cc:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
 buffer

On Tue, 6 Dec 2016 00:37:08 +0000
Long Li <longli@...rosoft.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@...workplumber.org]
> > Sent: Monday, December 5, 2016 8:53 AM
> > To: Long Li <longli@...rosoft.com>
> > Cc: KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> > <haiyangz@...rosoft.com>; Bjorn Helgaas <bhelgaas@...gle.com>;
> > devel@...uxdriverproject.org; linux-kernel@...r.kernel.org; linux-
> > pci@...r.kernel.org
> > Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> > buffer
> > 
> > On Tue,  8 Nov 2016 14:04:38 -0800
> > Long Li <longli@...hange.microsoft.com> wrote:
> >   
> > > +	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> > > +
> > > +	params = &hbus->retarget_msi_interrupt_params;
> > > +	memset(params, 0, sizeof(*params));
> > > +	params->partition_id = HV_PARTITION_ID_SELF;
> > > +	params->source = 1; /* MSI(-X) */
> > > +	params->address = msi_desc->msg.address_lo;
> > > +	params->data = msi_desc->msg.data;
> > > +	params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > >  			   (hbus->hdev->dev_instance.b[4] << 16) |
> > >  			   (hbus->hdev->dev_instance.b[7] << 8) |
> > >  			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > >  			   PCI_FUNC(pdev->devfn);
> > > -	params.vector = cfg->vector;
> > > +	params->vector = cfg->vector;
> > >
> > >  	for_each_cpu_and(cpu, dest, cpu_online_mask)
> > > -		params.vp_mask |= (1ULL <<  
> > vmbus_cpu_number_to_vp_number(cpu));  
> > > +		params->vp_mask |= (1ULL <<  
> > vmbus_cpu_number_to_vp_number(cpu));  
> > > +
> > > +	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> > >
> > > -	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, &params, NULL);
> > > +	spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);  
> > 
> > It looks like the additional locking here is being overly paranoid.
> > The caller is already holding the irq descriptor lock. Look at fixup_irqs.  
> 
> You are right. On my test machine, there are two possible places calling hv_irq_unmask(): request _irq() and handle_edge_irq(). They both have desc->lock held when calling .irq_unmask on the chip. A review of the IRQ code shows that desc->lock is always held while calling chip->irq_unmask().
> 
> Since the lock doesn't do any harm and it is not on performance code path, we can remove the lock in the upcoming patches.

Why add it then remove it, your patch hasn't been accepted. Please revise it and remove it.
Don't add additional unnecessary code.