From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A98192FE3A for ; Wed, 8 Nov 2023 23:11:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="BQ9bLrAA" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1DE37259B for ; Wed, 8 Nov 2023 15:11:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699485065; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7zBhCsl+J9XuFKJwNU+AckeR5uKVYWw4Qceb+g+hq/k=; b=BQ9bLrAAB5nwUNe06oq2Xt5qjWiyIOhRDgGEK+Rha/MmfuGzcQy3ag4656TxT35sfyWs3l KAr2M8BV2IAuefrTb6SfNaS4TNWVZmsPx2RHijdv4Mq70/9Tme8JJfZueeqTFC2VqUq1sN p4jE8I1DhJvJiNPx6TzhIa8lLK/xUa0= Received: from mail-ed1-f72.google.com (mail-ed1-f72.google.com [209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-384-B1SA-0YUML2q-3RJ1yX15g-1; Wed, 08 Nov 2023 18:11:03 -0500 X-MC-Unique: B1SA-0YUML2q-3RJ1yX15g-1 Received: by mail-ed1-f72.google.com with SMTP id 4fb4d7f45d1cf-543d2bc7d9dso110134a12.1 for ; Wed, 08 Nov 2023 15:11:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699485060; x=1700089860; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=7zBhCsl+J9XuFKJwNU+AckeR5uKVYWw4Qceb+g+hq/k=; b=wczQ6l0xS1uElhioz1gUCTOSo+TMxI5U+ebsb/YUPRnkZ33SXRYfA+AEnn3zx3Xd2u +UmBAFAR4Syq1iGwQghmwQZnUB/64KfMPsFCsxQTS7Ro3uC8d+AoK4FaKL5s8EHNVjm7 7nxvR41aNgwYYwN/s9mYcqirabvFJ7xbTyiZieh7Vc0UYY4hkv3Y9MV2lfH8y7r4j25n rmhFtfD3+hrBOqnkNOGun2tzjUUFs2cm/7hwLbjF9EIGnOBqJYtlfSBNJoqBI+vQ3Bs9 eVte/4LxJpyHUS8Q+BKOPIp/bZWjOTq12V+XGMIUDV6Ug/IqhbG6RsRhL34hGrt07700 wm+g== X-Gm-Message-State: AOJu0YxVksZTcKkt7HMdkMeWZuRccrs1wfqAvpIxn8dO76UxtQp4fqoR DyV6tCRcbUSFh6eCGdNjAV6KSopios1sXQJymoFgkkr4Izn7uTBzHRMFjfxsZc70po7j388hNEB 2fugZAl168Ow6QppJ X-Received: by 2002:a17:907:3f17:b0:9b3:308:d045 with SMTP id hq23-20020a1709073f1700b009b30308d045mr3130121ejc.46.1699485060542; Wed, 08 Nov 2023 15:11:00 -0800 (PST) X-Google-Smtp-Source: AGHT+IGgT5SEAwLzC5r/BVkOQGnVjCHOESssbnCbCYt42mXA7+rcpol8Z1ymofhy8h2p7zzk10z75A== X-Received: by 2002:a17:907:3f17:b0:9b3:308:d045 with SMTP id hq23-20020a1709073f1700b009b30308d045mr3130105ejc.46.1699485060207; Wed, 08 Nov 2023 15:11:00 -0800 (PST) Received: from alrua-x1.borgediget.toke.dk ([2a0c:4d80:42:443::2]) by smtp.gmail.com with ESMTPSA id e22-20020a1709067e1600b009ddaa2183d4sm1681156ejr.42.2023.11.08.15.10.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 Nov 2023 15:10:59 -0800 (PST) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 8E1EBEE6EBE; Thu, 9 Nov 2023 00:10:59 +0100 (CET) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: "Nelson, Shannon" , Jesper Dangaard Brouer , David Ahern , Jakub Kicinski , netdev@vger.kernel.org, bpf@vger.kernel.org, Daniel Borkmann , Alexei Starovoitov , Andrii Nakryiko Subject: Re: BPF/XDP: kernel panic when removing an interface that is an xdp_redirect target In-Reply-To: References: <87h6lxy3zq.fsf@toke.dk> X-Clacks-Overhead: GNU Terry Pratchett Date: Thu, 09 Nov 2023 00:10:59 +0100 Message-ID: <871qczx2m4.fsf@toke.dk> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable "Nelson, Shannon" writes: > On 11/7/2023 7:31 AM, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >>=20 >> "Nelson, Shannon" writes: >>=20 >>> While testing new code to support XDP in the ionic driver we found that >>> we could panic the kernel by running a bind/unbind loop on the target >>> interface of an xdp_redirect action. Obviously this is a stress test >>> that is abusing the system, but it does point to a window of opportunity >>> in bq_enqueue() and bq_xmit_all(). I believe that while the validity of >>> the target interface has been checked in __xdp_enqueue(), the interface >>> can be unbound by the time either bq_enqueue() or bq_xmit_all() tries to >>> use the interface. There is no locking or reference taken on the >>> interface to hold it in place before the target=E2=80=99s ndo_xdp_xmit(= ) is called. >>> >>> Below is a stack trace that our tester captured while running our test >>> code on a RHEL 9.2 kernel =E2=80=93 yes, I know, unpublished driver cod= e on a >>> non-upstream kernel. But if you look at the current upstream code in >>> kernel/bpf/devmap.c I think you can see what we ran into. >>> >>> Other than telling users to not abuse the system with a bind/unbind >>> loop, is there something we can do to limit the potential pain here? >>> Without knowing what interfaces might be targeted by the users=E2=80=99= XDP >>> programs, is there a step the originating driver can do to take >>> precautions? Did we simply miss a step in the driver, or is this an >>> actual problem in the devmap code? >>=20 >> Sounds like a driver bug :) > > Entirely possible, wouldn't be our first ... :-) > >>=20 >> The XDP redirect flow guarantees that all outstanding packets are >> flushed within a single NAPI cycle, as documented here: >> https://docs.kernel.org/bpf/redirect.html >>=20 >> So basically, the driver should be doing a two-step teardown: remove >> global visibility of the resource in question, wait for all concurrent >> users to finish, and *then* free the data structure. This corresponds to >> the usual RCU protection: resources should be kept alive until all >> concurrent RCU critical sections have exited on all CPUs. So if your >> driver is removing an interface's data structure without waiting for >> concurrent NAPI cycles to finish, that's a bug in the driver. >>=20 >> This kind of thing is what the synchronize_net() function is for; for a >> usage example, see veth_napi_del_range(). My guess would be that you're >> missing this as part of your driver teardown flow? > > Essentially, the first thing we do in the remove function is to call=20 > unregister_netdev(), which has synchronize_net() in the path, so I don't= =20 > think this is missing from our scenario, but thanks for the hint, I'll=20 > keep this in mind. I do see there are a couple of net drivers that are=20 > more aggressive about calling it directly in some other parts of the=20 > logic - I don't think that has a bearing on this issue, but I'll keep it= =20 > in mind. Hmm, right, in fact unregister_netdev() has two such synchronize_net() calls. The XDP queue is only guaranteed to be flushed after the second one of those, though, and there's an 'ndo_uninit()' callback in-between them. So I don't suppose your driver implements that ndo and does something there that could cause the crash you're seeing? Otherwise, the one thing I can think of is that maybe it can be related to the fact that synchronize_net() turns into a synchronize_rcu_expedited() if the rtnl lock is held (which it is in this case if you're calling the parameter-less unregister_netdev()). I'm not quite sure I grok the expedited wait thing, but it should be pretty easy to check if this is the cause by making a change like the one below and seeing if the issue goes away. -Toke diff --git a/net/core/dev.c b/net/core/dev.c index e28a18e7069b..1a035a5f0b0e 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10932,7 +10932,7 @@ void synchronize_net(void) { might_sleep(); if (rtnl_is_locked()) - synchronize_rcu_expedited(); + synchronize_rcu(); else synchronize_rcu(); }