linux-kernel - Re: [PATCH part1 v5 5/7] PCI: Add pci_dummy

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: <1392026834.2082.10.camel@linux-fkkt.site>
Date:	Mon, 10 Feb 2014 11:07:14 +0100
From:	Oliver Neukum <oneukum@...e.de>
To:	Yijing Wang <wangyijing@...wei.com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, Hanjun Guo <guohanjun@...wei.com>
Subject: Re: [PATCH part1 v5 5/7] PCI: Add pci_dummy_ops to isolate pci
 device temporarily

On Mon, 2014-02-10 at 15:59 +0800, Yijing Wang wrote:
> Hi Oliver,
>    Thanks for your review and comments!
> 
> >> +static DEFINE_SPINLOCK(pci_freeze_lock);
> > 
> > The lock is used only here.
> 
> Also be used in pci_bus_unfreeze_device();

Sorry, I meant only in this patch.

> 
> > 
> >> +/**
> >> + * pci_bus_freeze_device - freeze pci bus to access pci device
> >> + * @bus: the pci bus to freeze
> >> + *
> >> + * Replace pci bus ops by pci_dummy_ops, protect system from
> >> + * accessing pci devices.
> >> + */
> >> +void pci_bus_freeze_device(struct pci_bus *bus)
> >> +{
> >> +	struct pci_ops *ops;
> >> +	unsigned long flags;
> >> +
> >> +	spin_lock_irqsave(&pci_freeze_lock, flags);
> >> +	ops = pci_bus_set_ops(bus, &pci_dummy_ops);
> >> +	bus->save_ops = ops;
> >> +	spin_unlock_irqrestore(&pci_freeze_lock, flags);
> > 
> > Against what exactly are you locking here?
> 
> I want to use this spin lock to serialize freeze device and unfreeze device.

Yes, but against what? I am sorry I should have been more explicit.
You are using these functions only in pci_scan_single_device()


CPU A					CPU B
pci_bus_freeze_device()			wait
bus->save_ops = ops {valid}		wait
...					pci_bus_freeze_device()
wait					bus->save_ops = ops
					{pci_dummy_ops !}
pci_bus_unfreeze_device()		wait
pci_bus_set_ops(bus, bus->save_ops)

You see the problem?

If this function ever races with itself, the locking is useless.
If it doesn't race with itself, the locking is not needed.
If this function can really race with itself, you need a refcount
for freezing.

	Regards
		Oliver


--
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/