Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   "Ertman, David M" <david.m.ertman@intel.com>
To:     ivecera <ivecera@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
CC:     poros <poros@redhat.com>, mschmidt <mschmidt@redhat.com>,
        Leon Romanovsky <leon@kernel.org>,
        "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "Saleem, Shiraz" <shiraz.saleem@intel.com>,
        "moderated list:INTEL ETHERNET DRIVERS" 
        <intel-wired-lan@lists.osuosl.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net v3] ice: Fix race during aux device (un)plugging
Thread-Topic: [PATCH net v3] ice: Fix race during aux device (un)plugging
Thread-Index: AQHYVUZcHI6VX+DIGkiO7KkonUY3tKz8NYDQ
Date:   Fri, 22 Apr 2022 17:42:25 +0000
Message-ID: <MW5PR11MB581100DBD307763A92012BEADDF79@MW5PR11MB5811.namprd11.prod.outlook.com>
References: <20220421060906.1902576-1-ivecera@redhat.com>
In-Reply-To: <20220421060906.1902576-1-ivecera@redhat.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.401.20
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?n4jnOcRvYTS8GxYy20wGHYbjNB90oF257TVMybh0A5EfCNGOIsSFTdqJP2Lm?=
 =?us-ascii?Q?KIcX/ziO4NaSeGebCNCFWKx4836TZJejetTqswuPuKH5pRIqPN8CNat1rKwr?=
 =?us-ascii?Q?x5KBz8RBAQjqQhHY8XJius5opkCrVihkhh1EZu2s/SRt/jg/h8YTMax79dC1?=
 =?us-ascii?Q?6Ibodw7rA6vO1fsXv0r81Rqrdpg3J5kPQMENHxh0TjI3AanmF0VXGgGL8Nja?=
 =?us-ascii?Q?u/cIwiI78U4rFQS8uE+xtjWcUfOKGATIq3T6Dz5FQL7QlvL1LCJnk/AEvptd?=
 =?us-ascii?Q?ILp/TAQO70dXv5LzWNm+xLv7RcgfPCITUdILHFawF77sEpYyj4MWKDgWlYeC?=
 =?us-ascii?Q?+3bLpaOov6uqg4JI5zwjI1/1f1nW+DU++dgfmch5j6+5h6664WvCv124w5RM?=
 =?us-ascii?Q?MnSHjkYZlpXBpjui+tDUz+YUW8Us1f/ibu3bcUAoaZDR6+M8/jh/GU9jMMER?=
 =?us-ascii?Q?njIts4uue7UBnUg6WwnRUiCsolCoXAcprjHZx8rKAkERpQ/9H0vyrcpH9+gU?=
 =?us-ascii?Q?eaL7PxzvF+NYkuzHkkNnMHpfL+rkzw1RhUND2xwY6ezpL5K5xN8CvMEsNqHD?=
 =?us-ascii?Q?3X+AGEKrmfJ35km9T/jnsH0icSet32rDuWl5kLuEij+3lOn2+Ct76TcRQL1g?=
 =?us-ascii?Q?iKbBdlyBjwForGAo39GJilzDUxwFs3B+5V1P0nZlXDe+lnz3SqX9KrbTkB2q?=
 =?us-ascii?Q?EHdFnlDOqJhdWF0Y3YCzhF0xyXr7ABrcstjTQw7Un1JU6l4LWo7S+ZSdsmgG?=
 =?us-ascii?Q?P519M4uo8h+nXF4ynpXqLyOqCRTNkzuAEifBK19fgoZn0+TxEPhAtIFwt0Jn?=
 =?us-ascii?Q?BWCUAriGivcfQANM1MA7ixfMH+PrJEHxeRcYCwSv4Iq0rmcUgoV2ZxojmUhm?=
 =?us-ascii?Q?LfAKg0LLdsKh7hMg5o4V0EEj2hqH6Q2I5J756vJ+Mhi38UPTV7+5vEX/gwUT?=
 =?us-ascii?Q?BSv0+XuhhIixLvYOQHziZFZPU9XPbFmZQg+Yfe21bJRB0RfGAVowZvzOQqOQ?=
 =?us-ascii?Q?YGEp1CIoFmhj58uajbDuuj0BsYc0esKSL9yIW+EmhcYExQqPE2oeUnxm6q4Y?=
 =?us-ascii?Q?x413eJsY0U0dhak1oF99BKG4Tx6ZOzWRysi73Ig6CHzCU2evEnXTuony7Dob?=
 =?us-ascii?Q?psKsz/seOGpaySGybdBm1OGxMvReliTUaX2osS14pev9oDK7L5Q+AilbLjkW?=
 =?us-ascii?Q?+dhQcF2mfMBbjG1c/pHkJHsBvC3EXCuNHTN5sxAdEy8RguhN6zVWlf42rzRZ?=
 =?us-ascii?Q?KjPvaEqUSwWo7IBlhRzn9rrp5dO1V6SJoqUIbFBxbv9asjzxZeXR3iWW4uhm?=
 =?us-ascii?Q?f52otXqACz/DeTY42SEdu//7tD5CfLm0rCh41wfWYAkP9e0S2z+qs2lS8MfW?=
 =?us-ascii?Q?2w5WLNEFNnD9ZVMq6g+vHv6XPVz2b1taQXVPFWktA4B1F9CTt1S/B5XdyicN?=
 =?us-ascii?Q?gxNiSW4tETAPqlI52vwIRfY/vOtL+PK5jvwjORHSwOYzFyvH5QtAr0AR92YU?=
 =?us-ascii?Q?b9uUFdnteI5GZ33enAgRl9zmPQ5SS4yj5lmwAlunrXzEaTO9xKIi6EQ1wmVF?=
 =?us-ascii?Q?mv1Qbs/wMEIePe3pwE9nQ4ThKRN0PiagpdWwUndILo+a2dys+wkthcIouc8n?=
 =?us-ascii?Q?l5OTJmUlHa/ogWap5wQoyuGebgoPKh/n8asHUiR18p5arddivKhy/HcKZERq?=
 =?us-ascii?Q?GGsxafX0OXgu8YEzfcJaMdnb38ZwskRssVBQ+s1F7KNWarjmWuZNiTGPqF+H?=
 =?us-ascii?Q?g739cKbSDKx+aJhDcwjcs3sGobfdaJU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MW5PR11MB5811.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9896047d-efdd-44de-02b8-08da248776b0
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Apr 2022 17:42:25.6153
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: xNImXmsQ8zHLJWzplBctACf2BTs9l9m238KvRw6WOGeFT6lX7ldt7uQQfpE4IBib86D/M3tRZ3I9ollXYhwRhiSzjnUyS2wmA/bxWOICR4E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4722
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: Ivan Vecera <ivecera@redhat.com>
> Sent: Wednesday, April 20, 2022 11:09 PM
> To: netdev@vger.kernel.org
> Cc: poros <poros@redhat.com>; mschmidt <mschmidt@redhat.com>; Leon
> Romanovsky <leon@kernel.org>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> Ertman, David M <david.m.ertman@intel.com>; Saleem, Shiraz
> <shiraz.saleem@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-
> wired-lan@lists.osuosl.org>; open list <linux-kernel@vger.kernel.org>
> Subject: [PATCH net v3] ice: Fix race during aux device (un)plugging
>=20
> Function ice_plug_aux_dev() assigns pf->adev field too early prior
> aux device initialization and on other side ice_unplug_aux_dev()
> starts aux device deinit and at the end assigns NULL to pf->adev.
> This is wrong because pf->adev should always be non-NULL only when
> aux device is fully initialized and ready. This wrong order causes
> a crash when ice_send_event_to_aux() call occurs because that function
> depends on non-NULL value of pf->adev and does not assume that
> aux device is half-initialized or half-destroyed.
> After order correction the race window is tiny but it is still there,
> as Leon mentioned and manipulation with pf->adev needs to be protected
> by mutex.
>=20
> Fix (un-)plugging functions so pf->adev field is set after aux device
> init and prior aux device destroy and protect pf->adev assignment by
> new mutex. This mutex is also held during ice_send_event_to_aux()
> call to ensure that aux device is valid during that call. Device
> lock used ice_send_event_to_aux() to avoid its concurrent run can
> be removed as this is secured by that mutex.
>=20
> Reproducer:
> cycle=3D1
> while :;do
>         echo "#### Cycle: $cycle"
>=20
>         ip link set ens7f0 mtu 9000
>         ip link add bond0 type bond mode 1 miimon 100
>         ip link set bond0 up
>         ifenslave bond0 ens7f0
>         ip link set bond0 mtu 9000
>         ethtool -L ens7f0 combined 1
>         ip link del bond0
>         ip link set ens7f0 mtu 1500
>         sleep 1
>=20
>         let cycle++
> done
>=20
> In short when the device is added/removed to/from bond the aux device
> is unplugged/plugged. When MTU of the device is changed an event is
> sent to aux device asynchronously. This can race with (un)plugging
> operation and because pf->adev is set too early (plug) or too late
> (unplug) the function ice_send_event_to_aux() can touch uninitialized
> or destroyed fields. In the case of crash below pf->adev->dev.mutex.
>=20
> Crash:
> [   53.372066] bond0: (slave ens7f0): making interface the new active one
> [   53.378622] bond0: (slave ens7f0): Enslaving as an active interface wi=
th an u
> p link
> [   53.386294] IPv6: ADDRCONF(NETDEV_CHANGE): bond0: link becomes
> ready
> [   53.549104] bond0: (slave ens7f1): Enslaving as a backup interface wit=
h an
> up
>  link
> [   54.118906] ice 0000:ca:00.0 ens7f0: Number of in use tx queues change=
d
> inval
> idating tc mappings. Priority traffic classification disabled!
> [   54.233374] ice 0000:ca:00.1 ens7f1: Number of in use tx queues change=
d
> inval
> idating tc mappings. Priority traffic classification disabled!
> [   54.248204] bond0: (slave ens7f0): Releasing backup interface
> [   54.253955] bond0: (slave ens7f1): making interface the new active one
> [   54.274875] bond0: (slave ens7f1): Releasing backup interface
> [   54.289153] bond0 (unregistering): Released all slaves
> [   55.383179] MII link monitoring set to 100 ms
> [   55.398696] bond0: (slave ens7f0): making interface the new active one
> [   55.405241] BUG: kernel NULL pointer dereference, address:
> 0000000000000080
> [   55.405289] bond0: (slave ens7f0): Enslaving as an active interface wi=
th an u
> p link
> [   55.412198] #PF: supervisor write access in kernel mode
> [   55.412200] #PF: error_code(0x0002) - not-present page
> [   55.412201] PGD 25d2ad067 P4D 0
> [   55.412204] Oops: 0002 [#1] PREEMPT SMP NOPTI
> [   55.412207] CPU: 0 PID: 403 Comm: kworker/0:2 Kdump: loaded Tainted: G
> S
>            5.17.0-13579-g57f2d6540f03 #1
> [   55.429094] bond0: (slave ens7f1): Enslaving as a backup interface wit=
h an
> up
>  link
> [   55.430224] Hardware name: Dell Inc. PowerEdge R750/06V45N, BIOS 1.4.4
> 10/07/
> 2021
> [   55.430226] Workqueue: ice ice_service_task [ice]
> [   55.468169] RIP: 0010:mutex_unlock+0x10/0x20
> [   55.472439] Code: 0f b1 13 74 96 eb e0 4c 89 ee eb d8 e8 79 54 ff ff 6=
6 0f 1f 84
> 00 00 00 00 00 0f 1f 44 00 00 65 48 8b 04 25 40 ef 01 00 31 d2 <f0> 48 0f=
 b1 17 75
> 01 c3 e9 e3 fe ff ff 0f 1f 00 0f 1f 44 00 00 48
> [   55.491186] RSP: 0018:ff4454230d7d7e28 EFLAGS: 00010246
> [   55.496413] RAX: ff1a79b208b08000 RBX: ff1a79b2182e8880 RCX:
> 0000000000000001
> [   55.503545] RDX: 0000000000000000 RSI: ff4454230d7d7db0 RDI:
> 0000000000000080
> [   55.510678] RBP: ff1a79d1c7e48b68 R08: ff4454230d7d7db0 R09:
> 0000000000000041
> [   55.517812] R10: 00000000000000a5 R11: 00000000000006e6 R12:
> ff1a79d1c7e48bc0
> [   55.524945] R13: 0000000000000000 R14: ff1a79d0ffc305c0 R15:
> 0000000000000000
> [   55.532076] FS:  0000000000000000(0000) GS:ff1a79d0ffc00000(0000)
> knlGS:0000000000000000
> [   55.540163] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   55.545908] CR2: 0000000000000080 CR3: 00000003487ae003 CR4:
> 0000000000771ef0
> [   55.553041] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [   55.560173] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [   55.567305] PKRU: 55555554
> [   55.570018] Call Trace:
> [   55.572474]  <TASK>
> [   55.574579]  ice_service_task+0xaab/0xef0 [ice]
> [   55.579130]  process_one_work+0x1c5/0x390
> [   55.583141]  ? process_one_work+0x390/0x390
> [   55.587326]  worker_thread+0x30/0x360
> [   55.590994]  ? process_one_work+0x390/0x390
> [   55.595180]  kthread+0xe6/0x110
> [   55.598325]  ? kthread_complete_and_exit+0x20/0x20
> [   55.603116]  ret_from_fork+0x1f/0x30
> [   55.606698]  </TASK>
>=20
> Fixes: f9f5301e7e2d ("ice: Register auxiliary device to provide RDMA")
> Cc: Leon Romanovsky <leon@kernel.org>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>

Sorry for previous mis-reply - hit the wrong button.

LGTM
Acked-by: Dave Ertman <david.m.ertman@intel.com>