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]

Message-Id: <201202291719.50764.jackm@dev.mellanox.co.il>
Date:	Wed, 29 Feb 2012 17:19:50 +0200
From:	Jack Morgenstein <jackm@....mellanox.co.il>
To:	David Miller <davem@...emloft.net>, cascardo@...ux.vnet.ibm.com,
	yevgenyp@...lanox.co.il, roland@...estorage.com
Cc:	netdev@...r.kernel.org, linux-rdma@...r.kernel.org
Subject: Re: [PATCH] mlx4: prevent the device from being removed concurrently

On Wednesday 29 February 2012 16:47, Jack Morgenstein wrote:
> Some comments.
> 
> 1. Mr Cascardo's solution is only partial, and does not cover all the problem cases. He
>    simply uncovered one of several examples of what lack-of-sync will do when removing a device.
>    Mr. Cascardo found the kernel Oops that happens when a catastrophic error occurs during device
>    removal. What if we receive a catas error while doing "init_one"?  What if we are in the middle
>    of catas error recovery (in the init_one stage), and we get a remove_one request from higher up?
> 
>    There is a solution for this precise problem in the mthca driver (infiniband/hw/mthca/mthca_main.c
>    infiniband/hw/mthca/mthca_catas.c). In the mthca driver, we DO in fact use an "mthca_device_mutex"
>    for precisely the reason given in a. above.  I see no reason not to do the same thing here.
> 
>    This requires:
> 	1. mlx4_init_one(), mlx4_remove_one() and mlx4_restart_one all grab an mlx4_device_mutex.
>         2. new procedure __mlx4_remove_one(), which does not grab the mutex.
> 
>    Note that it is NOT enough to simply protect the removal operation.  The protection must wrap the
>    ENTIRE restart operation (both removal and init), because allowing a remove in the middle of init_one
>    or restart_one would probably also cause a kernel Oops.
> 
> 2. The intf_mutex is used with mlx4_un/register_device and mlx4_un/register_interface. unregister_device is
>    used both in remove_one and in mlx4_change_port_types.  I would hesitate to grab that mutex for a more
>    global use.  I think it is cleaner to add a device mutex (mlx4_device_mutex) for initializing/removing/
>    restarting the device.
> 
> -Jack
> 
Another thing -- what about the desired final state of the device?  Say we do a remove one, and in the middle
of this, we get a catas restart.  The catas restart will wait until the remove-in-progress completes, and
then will do its remove/init -- with the net result that the device is UP rather than DOWN.

This implies that we need some sort of state machine here.

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