From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CBB782206AC for ; Thu, 27 Feb 2025 13:31:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740663067; cv=none; b=Q17hO/+tzeyT/OZMKnV5jVQJA6+JtY/a8ws6uuR5B5bogF5zz3La1RmtgAMM5KLxEIEpMJ0EVN9aZgjwKKih++8t+36mtIYzdTio6g75+Tk9p3teWQTAGBnBry0jjKa++51qyludODjIny6F7c0QP65frlkKjL7/EzS63sjWeaI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740663067; c=relaxed/simple; bh=5K1DQgpCnV8OGVSz6GIqpEfTt8kjItGXwWAZ4X3AWLM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=qm73SVt5p92s8OamMee4cPaUer8Ws2Vf46Xk+tHCWxNFbsOQ41TN3mh/EkxFTq5reVcVREc6ZmD7OevJKV/sVzE7k3geJJctKV7ermRs3Vr0Xr0NXoDR0PlvQc03WtRMbp/08BUrh6rnxreL5zP9qGAQax/quj1fxASRgtlSqoA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=blackwall.org; spf=none smtp.mailfrom=blackwall.org; dkim=pass (2048-bit key) header.d=blackwall-org.20230601.gappssmtp.com header.i=@blackwall-org.20230601.gappssmtp.com header.b=ihRs/256; arc=none smtp.client-ip=209.85.208.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=blackwall.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=blackwall.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=blackwall-org.20230601.gappssmtp.com header.i=@blackwall-org.20230601.gappssmtp.com header.b="ihRs/256" Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-5e04f2b1685so1195062a12.0 for ; Thu, 27 Feb 2025 05:31:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=blackwall-org.20230601.gappssmtp.com; s=20230601; t=1740663064; x=1741267864; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=7wXFTKX1544/JIF02ehdLe2SrnajJcR6+uQqWAAhPPY=; b=ihRs/256BSg5Z5Cbra1h9RobjQnf2t4kdPCyir05PZiMdHHq2oLTr9+YSTW2Uxs6a/ gprp/a+hdm2GYPbvkQZbSPKMOJ208igladzRPsXOF/+5oE6KnU+hy3oNqQ4Fo5stVxOX UsEB92c8vXgcvgnlTbuKBOgiJidholdphkNBvweODGLqY9EkSzUx/Cbl/jGWqcSbafWy iI2HFPvWEJXsEBGDS7v3+20X1//GKO7PokvUK11CmDnJB+rjSkNEk3ehgUg2iBRlZnHT qLTnjIhXJYAAhMueyiZeni/wQoSlXZDOX7ordoirjSy3tCOCyHvjxouD4XgyY6lvWgy/ ZEZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740663064; x=1741267864; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7wXFTKX1544/JIF02ehdLe2SrnajJcR6+uQqWAAhPPY=; b=kJ9E7DIATds8hW3ASs5Q9DyVVC4B6X/L07xX/itI2846j8gjDpQoo2xo+XbcJ5Xi3L 0eMAFaq9yjRjHGHu6O8PKfu39FMydJL519FnTM8I5XYY8ofTbfFmZsE+CR/RnQ78BMkf tnyVSb3SsJ6k4HwH2adQ1dr42SVza89ZJBBEUVPQxanSo/w6vgOXBQc9j3SGaZe2Qz+b C4wbulPc5xv2cP/aPiDFYXIll1i/Cd/vX9C3jh5R99VWmDC6IR8VKCoGDrybDRxdZLnv 2gwqKx4PRRgS+3KqQ2LOE9M0fwAHJwx6W8GQDmCxZ7idEAkbb6C5WKYj/6VMSpn+lQhc pH+Q== X-Forwarded-Encrypted: i=1; AJvYcCU2+uYr7FsE4M+vQImkifF7IZnauKVOHnpeGQApFRS5stVpK6u5x7tFwZmnv+hOKxTmYI178M2OXVZzpM4=@vger.kernel.org X-Gm-Message-State: AOJu0YzfMBaDGcWIf936tQ9lTLunwpT99FRL3CxsbHQSyRqcQYNpbNW4 sBKJiBWCsRX6yorjttYdB65TMwXKf9LL+yK88707TJVnSvn98s2ds/9hMo/o4ww= X-Gm-Gg: ASbGncssPo+HMHlAJpnREdx8O+/pwKSALcMZmf7CydGz+SW4dPXLSnTYrPwhpibktxR lBUkJhuAvXQwt+7kXKHKvuJBdsSL83woPt7NbIAHM0wKZUn7iJMp6HEZnSQDbWVcpXYxTc/gt8q +nloRXpz+2WjHniRvIbCS79MaIitp1nTVAc0ubJ8oSgHOupRcJLd1PgZSLweXkPjC2lWyQa1145 Z1WeuRLgx9+1hB4EQDJzhZik5gD6qH8spcQ4GLWpypG5cyMnGHmdLpAUOjrwdXPyIWcl22KHdRg pHWqhPHoFIxZC1pn+0xzyXvngej4S9fEZtY6lwvdPcO7g8BGyLpYhH+GNA== X-Google-Smtp-Source: AGHT+IE0aNnzvv1Qi4koQK1Hzo4GjpUtdykg2u0L4Pn0RkAUHo9vRWuUm6DYMvxfLSo0bViwlkQMRQ== X-Received: by 2002:a05:6402:2553:b0:5de:594d:e9aa with SMTP id 4fb4d7f45d1cf-5e4455c30f9mr28084142a12.8.1740663063527; Thu, 27 Feb 2025 05:31:03 -0800 (PST) Received: from [192.168.0.205] (78-154-15-142.ip.btc-net.bg. [78.154.15.142]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf0b86e84csm125023766b.0.2025.02.27.05.31.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 27 Feb 2025 05:31:03 -0800 (PST) Message-ID: Date: Thu, 27 Feb 2025 15:31:01 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCHv3 net 1/3] bonding: move IPsec deletion to bond_ipsec_free_sa To: Hangbin Liu Cc: netdev@vger.kernel.org, Jay Vosburgh , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Shuah Khan , Tariq Toukan , Jianbo Liu , Jarod Wilson , Steffen Klassert , Cosmin Ratiu , linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org References: <20250227083717.4307-1-liuhangbin@gmail.com> <20250227083717.4307-2-liuhangbin@gmail.com> <446e8ef4-7ac0-43ad-99ff-29c21a2ee117@blackwall.org> <13cb4b16-51b0-4042-8435-6dac72586e55@blackwall.org> Content-Language: en-US From: Nikolay Aleksandrov In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 2/27/25 15:21, Hangbin Liu wrote: > On Thu, Feb 27, 2025 at 11:21:51AM +0200, Nikolay Aleksandrov wrote: >>>> @@ -617,6 +611,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) >>>> >>>> mutex_lock(&bond->ipsec_lock); >>>> list_for_each_entry(ipsec, &bond->ipsec_list, list) { >>>> + if (ipsec->xs->km.state == XFRM_STATE_DEAD) { >>>> + list_del(&ipsec->list); >>> >>> To be able to do this here, you'll have to use list_for_each_entry_safe(). >>> >> >> One more thing - note I'm not an xfrm expert by far but it seems to me here you have >> to also call xdo_dev_state_free() with the old active slave dev otherwise that will >> never get called with the original real_dev after the switch to a new >> active slave (or more accurately it might if the GC runs between the switching >> but it is a race), care must be taken wrt sequence of events because the XFRM > > Can we just call xs->xso.real_dev->xfrmdev_ops->xdo_dev_state_free(xs) > no matter xs->xso.real_dev == real_dev or not? I'm afraid calling > xdo_dev_state_free() every where may make us lot more easily. > You'd have to check all drivers that implement the callback to answer that and even then I'd stick to the canonical way of how it's done in xfrm and make the bond just passthrough. Any other games become dangerous and new code will have to be carefully reviewed every time, calling another device's free_sa when it wasn't added before doesn't sound good. >> GC may be running in parallel which probably means that in bond_ipsec_free_sa() >> you'll have to take the mutex before calling xdo_dev_state_free() and check >> if the entry is still linked in the bond's ipsec list before calling the free_sa >> callback, if it isn't then del_sa_all got to it before the GC and there's nothing >> to do if it also called the dev's free_sa callback. The check for real_dev doesn't >> seem enough to protect against this race. > > I agree that we need to take the mutex before calling xdo_dev_state_free() > in bond_ipsec_free_sa(). Do you think if this is enough? I'm a bit lot here. > > Thanks > Hangbin Well, the race is between the xfrm GC and del_sa_all, in bond's free_sa if you walk the list under the mutex before calling real_dev's free callback and don't find the current element that's being freed in free_sa then it was cleaned up by del_sa_all, otherwise del_sa_all is waiting to walk that list and clean the entries. I think it should be fine as long as free_sa was called once with the proper device.