Date:   Wed, 8 Mar 2023 13:04:50 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Lin Ma <linma@zju.edu.cn>
Cc:     jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, richardcochran@gmail.com,
        ast@kernel.org, daniel@iogearbox.net, hawk@kernel.org,
        john.fastabend@gmail.com, intel-wired-lan@lists.osuosl.org,
        pmenzel@molgen.mpg.de, regressions@lists.linux.dev,
        vinschen@redhat.com, stable@vger.kernel.org
Subject: Re: [PATCH v2] igb: revert rtnl_lock() that causes deadlock
Message-ID: <ZAh54ux2b6zS2kJY@corigine.com>
References: <ZAc/3oVos9DBx3iR@calimero.vinschen.de>
 <20230307152917.32182-1-linma@zju.edu.cn>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230307152917.32182-1-linma@zju.edu.cn>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1d3A4WRlDNGNuUBDbDNQkvKx7RZdAg50G030JFEzUJ4+VVYXdtyhZQ5Na42a?=
 =?us-ascii?Q?oJe7b5GAgb8zkq58BovldFylOWNsmJIU0A2TGU2txFj82iJVpyUCPzp4A8FQ?=
 =?us-ascii?Q?yVatso1GDOSdfvBFqXhA47oEIo4HgZgUrbSDg8P/kwO1JdXp4RSO5rVfFpFk?=
 =?us-ascii?Q?rOAHHVaT+0zYc8RwZ4sUusSSamJHI1RVHm6LLEeL5RgSun6ipn9rf3dnULdi?=
 =?us-ascii?Q?iEVgUKX7+znpL251wQdKFsOjORJFLc274o6Mh9Qy/UcDU7ueOAgWHOKLjMqw?=
 =?us-ascii?Q?n/XngMeTDfBvTmO/+5uUyP6by7Swx24cP3xDjgyZ9n35nMnBi7xPQ70TpMkG?=
 =?us-ascii?Q?Zf3zKLlloKLe2ZB/iAUL92gVcU7CHeKQVSKc4JmM16UWlzg8t+zc4C6EZ4tP?=
 =?us-ascii?Q?9YyhtU0mtbOvsUYFbrwMFmqNcZ+EJeN+xTxdpQMYs+9sr73gfRh7rjBUew3J?=
 =?us-ascii?Q?e9hOZBJhg4X09rIhG9yuDPCHywrIWwoA86K0fFCpeaaVl3KitUP75xoKpx0R?=
 =?us-ascii?Q?wTL18S8ML03KUgqgICvabhEffO12DSf/4WVGLgFo7yI4NX/gGwhFZbH5TIMJ?=
 =?us-ascii?Q?GdG7ecy0SVCFPZEz09Pbm/BihmQc9oqrLM+3xLY+0/g86SgF+Br+GycoVJuX?=
 =?us-ascii?Q?Fw6Pp8Vii3cCB37nra5le1nlo59zS43qpLmxNftbwjMwgJc/XtNmM1K3nWFb?=
 =?us-ascii?Q?KVH2FlD+RA/RFtEf5vm6TjjfXpOHtb5PGGtWo1wzKUGEuzYNu9wh6s+tGzwB?=
 =?us-ascii?Q?StTuvyqh/9yWfZWvaQ6+jn4wgWpWlZST9VsCW3NlJB/eqbz6pFBSt4UzSKvI?=
 =?us-ascii?Q?czZ+ydm/TQvHgzENiz2ZB0w69AW02TQrzXu82cAwNS88D95J8zXkt4+elrVZ?=
 =?us-ascii?Q?udsJmIeE8xAuZvtBNX6+SbVrd1Gn75KOv//8cw+G3ZqBx9suptpTcMnSqDGe?=
 =?us-ascii?Q?9xIolJeVFFycw1SKFAAwo967FWQOeqqMgUL/h2sEcJyrLUabjowaVxMn6Oxv?=
 =?us-ascii?Q?p+gQPPCxYWM0jip5sm5VzBerK7RCTS1xe32yiipCRWJ1typ3+MK1uYUTtuu6?=
 =?us-ascii?Q?1ShaspY6eNoFHznUkVMDUKET9HXieW5cUUmbQsaUO8y+iYUMp7hZjjGOKCXJ?=
 =?us-ascii?Q?jTQRHdYjjaDFOuxhfgTxTaQS4hQrdJxWvedyd44NQZeHUGtMIHV3HDg/htZn?=
 =?us-ascii?Q?bRQfrL1ikgF4hG7dfSd1eK3o+8rIhqYgkzTVa+d9bIW74pRXis7BUBKmPnoN?=
 =?us-ascii?Q?S8nPe/4uGKu32jZBBrcXbnNT7T2zZ7wbYnAh55PUmvvzVY9NUMIIEHXrHQJl?=
 =?us-ascii?Q?E7WqWLk4Qk4CAVGJdA4JdWsfYxc9/wqIEcULQWRNXTS27RRVGpgUXxKapatD?=
 =?us-ascii?Q?jxX81+ZIIzT3trpgk2o5Fv/pRCptedCNLdXJrCahdE4zYx1i366Y+Er3yu0c?=
 =?us-ascii?Q?qS9vv1qvvE+nRyWW3msTNNjoDa/drDTBr2OzDA1IkyGuAMEPNXq9tctVrw4s?=
 =?us-ascii?Q?uO/s/N4qr9Lb7LkMxbDBa4RDeTNU2OcZ7G4HqdjJnPD5vcWnX+ueLqTvyD7W?=
 =?us-ascii?Q?Rdz06HA9R5EN5jpUhLXtV38xi6t0CfQzxaTO/4obHxMHJulwbo7gmdKSKVQE?=
 =?us-ascii?Q?4Dk1N5QkO3Xb4qLY2gZpbLapP/0UGoYmzoJ9DaZRGnT/obrmLTsS942AaV7Q?=
 =?us-ascii?Q?i6HlzA=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 43a553f2-3f69-40f4-a201-08db1fcd56cf
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2023 12:04:58.9668
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /7fAnAwtRIoMXVc3PakCR5pPUJBE20HBFqAjPZsuqOKUCnMeZhrdwGxc7JT2o+GMzKfGhLtF0Sg86pdQnFcjY1stc1Peq+x+jkJlYpoUjho=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR13MB5552
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Mar 07, 2023 at 11:29:17PM +0800, Lin Ma wrote:
> The commit 6faee3d4ee8b ("igb: Add lock to avoid data race") adds
> rtnl_lock to eliminate a false data race shown below
> 
>  (FREE from device detaching)      |   (USE from netdev core)
> igb_remove                         |  igb_ndo_get_vf_config
>  igb_disable_sriov                 |  vf >= adapter->vfs_allocated_count?
>   kfree(adapter->vf_data)          |
>   adapter->vfs_allocated_count = 0 |
>                                    |    memcpy(... adapter->vf_data[vf]
> 
> The above race will never happen and the extra rtnl_lock causes deadlock
> below
> 
> [  141.420169]  <TASK>
> [  141.420672]  __schedule+0x2dd/0x840
> [  141.421427]  schedule+0x50/0xc0
> [  141.422041]  schedule_preempt_disabled+0x11/0x20
> [  141.422678]  __mutex_lock.isra.13+0x431/0x6b0
> [  141.423324]  unregister_netdev+0xe/0x20
> [  141.423578]  igbvf_remove+0x45/0xe0 [igbvf]
> [  141.423791]  pci_device_remove+0x36/0xb0
> [  141.423990]  device_release_driver_internal+0xc1/0x160
> [  141.424270]  pci_stop_bus_device+0x6d/0x90
> [  141.424507]  pci_stop_and_remove_bus_device+0xe/0x20
> [  141.424789]  pci_iov_remove_virtfn+0xba/0x120
> [  141.425452]  sriov_disable+0x2f/0xf0
> [  141.425679]  igb_disable_sriov+0x4e/0x100 [igb]
> [  141.426353]  igb_remove+0xa0/0x130 [igb]
> [  141.426599]  pci_device_remove+0x36/0xb0
> [  141.426796]  device_release_driver_internal+0xc1/0x160
> [  141.427060]  driver_detach+0x44/0x90
> [  141.427253]  bus_remove_driver+0x55/0xe0
> [  141.427477]  pci_unregister_driver+0x2a/0xa0
> [  141.428296]  __x64_sys_delete_module+0x141/0x2b0
> [  141.429126]  ? mntput_no_expire+0x4a/0x240
> [  141.429363]  ? syscall_trace_enter.isra.19+0x126/0x1a0
> [  141.429653]  do_syscall_64+0x5b/0x80
> [  141.429847]  ? exit_to_user_mode_prepare+0x14d/0x1c0
> [  141.430109]  ? syscall_exit_to_user_mode+0x12/0x30
> [  141.430849]  ? do_syscall_64+0x67/0x80
> [  141.431083]  ? syscall_exit_to_user_mode_prepare+0x183/0x1b0
> [  141.431770]  ? syscall_exit_to_user_mode+0x12/0x30
> [  141.432482]  ? do_syscall_64+0x67/0x80
> [  141.432714]  ? exc_page_fault+0x64/0x140
> [  141.432911]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> 
> Since the igb_disable_sriov() will call pci_disable_sriov() before
> releasing any resources, the netdev core will synchronize the cleanup to
> avoid any races. This patch removes the useless rtnl_(un)lock to guarantee
> correctness.
> 
> CC: stable@vger.kernel.org
> Fixes: 6faee3d4ee8b ("igb: Add lock to avoid data race")
> Reported-by: Corinna Vinschen <vinschen@redhat.com>
> Link: https://lore.kernel.org/intel-wired-lan/ZAcJvkEPqWeJHO2r@calimero.vinschen.de/
> Signed-off-by: Lin Ma <linma@zju.edu.cn>

Reviewed-by: Simon Horman <simon.horman@corigine.com>