netdev - Re: [PATCH] mlx4: prevent the device from being removed concurrently

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]

Date:	Tue, 28 Feb 2012 17:34:38 -0300
From:	Thadeu Lima de Souza Cascardo <cascardo@...ux.vnet.ibm.com>
To:	David Miller <davem@...emloft.net>
Cc:	yevgenyp@...lanox.co.il, netdev@...r.kernel.org,
	linux-rdma@...r.kernel.org, roland@...estorage.com,
	jackm@....mellanox.co.il
Subject: Re: [PATCH] mlx4: prevent the device from being removed concurrently

On Tue, Feb 28, 2012 at 02:30:51PM -0500, David Miller wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@...ux.vnet.ibm.com>
> Date: Tue, 28 Feb 2012 15:36:16 -0300
> 
> > When a EEH happens, the catas poll code will try to restart the device,
> > removing it and adding it back again. The EEH code will try to do the
> > same. One of the threads ends up accessing memory that was freed by the
> > other thread and we get a crash.
> 
> Stop adding bandaids to the locking.
> 
> If the EEH infrastructure doesn't synchronize parallel operations
> on the same device, that is the real bug, and that's where the real
> fix belongs.
> 
> I refuse to apply this patch.
> 

It's not EEH that does not synchronize removal. The problem is that the
driver itself calls the driver remove function through mlx4_restart_one.

>From catas.c:

 88 static void catas_reset(struct work_struct *work)
...
103                 ret = mlx4_restart_one(priv->dev.pdev);


>From main.c:

2013 int mlx4_restart_one(struct pci_dev *pdev)
...
2015         mlx4_remove_one(pdev);

2067 static struct pci_driver mlx4_driver = {
...
2071         .remove         = __devexit_p(mlx4_remove_one)


Real EEH support in this driver will have to do something similar to
reset the device. And this either requires that we remove or rewrite the
catas code, or that we implement some kind of locking.

Probably, what we should do here is rewrite catas code so that is
resilient to races with any code removing the device, be it EEH or
anything else. It's just that EEH will trigger the catas code and makes
it a lot easier to test this problem.

Regards.
Cascardo.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html