RE: [EXTERNAL] [PATCH 1/2] PCI: hv: Use IDR to generate transaction IDs for VMBus hardening

[Saurabh Singh Sengar <ssengar@microsoft.com>]

> -----Original Message-----
> From: Andrea Parri <parri.andrea@gmail.com>
> Sent: 19 March 2022 21:29
> To: Saurabh Singh Sengar <ssengar@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui
> <decui@microsoft.com>; Michael Kelley (LINUX) <mikelley@microsoft.com>;
> Wei Hu <weh@microsoft.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Wilczynski <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; linux-
> pci@vger.kernel.org; linux-hyperv@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [EXTERNAL] [PATCH 1/2] PCI: hv: Use IDR to generate transaction
> IDs for VMBus hardening
> 
> > > @@ -1208,6 +1211,27 @@ static void hv_pci_read_config_compl(void
> > > *context, struct pci_response *resp,
> > >  	complete(&comp->comp_pkt.host_event);
> > >  }
> > >
> > > +static inline int alloc_request_id(struct hv_pcibus_device *hbus,
> > > +				   void *ptr, gfp_t gfp)
> > > +{
> > > +	unsigned long flags;
> > > +	int req_id;
> > > +
> > > +	spin_lock_irqsave(&hbus->idr_lock, flags);
> > > +	req_id = idr_alloc(&hbus->idr, ptr, 1, 0, gfp);
> >
> > [Saurabh Singh Sengar] Many a place we are using alloc_request_id with
> GFP_KERNEL, which results this allocation inside of spin lock with
> GFP_KERNEL.
> 
> That's a bug.
> 
> 
> > Is this a good opportunity to use idr_preload ?
> 
> I'd rather fix (and 'simplify' a bit the interface) by doing:
> 
> static inline int alloc_request_id(struct hv_pcibus_device *hbus, void *ptr)
> {
> 	unsigned long flags;
> 	int req_id;
> 
> 	spin_lock_irqsave(&hbus->idr_lock, flags);
> 	req_id = idr_alloc(&hbus->idr, ptr, 1, 0, GFP_ATOMIC);
> 	spin_unlock_irqrestore(&hbus->idr_lock, flags);
> 	return req_id;
> }
> 
> Thoughts?
[Saurabh Sengar] Yes, if we are fine to use GFP_ATOMIC, this makes perfect sense.
Once fixed, please add: Reviewed-by: Saurabh Sengar <ssengar@microsoft.com>

> 
> Thanks,
>   Andrea