From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dl1-f46.google.com (mail-dl1-f46.google.com [74.125.82.46]) (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 88CAC3A35AC for ; Tue, 20 Jan 2026 21:04:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.46 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768943091; cv=none; b=Rkxj96SjWBE0FscZxhP5P8G7RKceC4Drx0PhAWpaGFSNbPIXBMBGgqsiurk+Lr3LKpJxPbIgb7Pvj+D+PVCQTP5mSjN2vZy4rWQ1NoQUsiHsLOv2iVqD42cqlTTwMJ9CwmNxmq14P/5GdG/JKee8rgqRztdryxbnC7nifoMqk8E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768943091; c=relaxed/simple; bh=UXP7g/BBPk/YWgk0/upubrDKRWdkIInmrHekkfzLkNo=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=tTq7VasjLEWwvWtWcbTa3DIGb/unZwKkO2tW8M6HXMBRNyupPiC9xWtYG6EemLwaoSsxiU2t/FUEXDAO865hHbqKfbfFIBeieYzkPaBMq0D2+rCS9OpB4lx6cWWG980sNpxEug7Rk4if71qS02OzbHBxfgldVxwU5TWrf6aHXfs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=mojatatu.com; spf=none smtp.mailfrom=mojatatu.com; dkim=pass (2048-bit key) header.d=mojatatu-com.20230601.gappssmtp.com header.i=@mojatatu-com.20230601.gappssmtp.com header.b=QQDzjRg+; arc=none smtp.client-ip=74.125.82.46 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=mojatatu.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=mojatatu.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=mojatatu-com.20230601.gappssmtp.com header.i=@mojatatu-com.20230601.gappssmtp.com header.b="QQDzjRg+" Received: by mail-dl1-f46.google.com with SMTP id a92af1059eb24-11f36012fb2so8489408c88.1 for ; Tue, 20 Jan 2026 13:04:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mojatatu-com.20230601.gappssmtp.com; s=20230601; t=1768943088; x=1769547888; 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=Ai1otIKDYCwf2Y8TFssFgbpLXbDAkZ98nVLTsrDMfKQ=; b=QQDzjRg+EfNb9xU2noNNQVPU9wAh+1jsVztkHkbzYogqHdQeZPHpCd1eH7tBsYLmX/ 9RztlgV8QwqUmcdGJK6wdlxY8qta/xwv/1KdSOyoz0E6yd/YIVcmcVRCIFKKFobyQc2I JkwYLN5ugRCTL+uuJmjxjWXr4NA6TjTb32FWf5cy2ZQL0gtRogGZd9oV1ehQUdZLeABK k+8Ec8C9pCigtApIEYZHA6iR+Jab4xy98uIjQi6a78WA9mqs7ZmnxWDb4dJkocmw6VhT 3UggRPWVbidqmqzhqPc1BQ5pli9ZuCFr6kYhkRr2OuarTxyi7Gk1DPXZGeEK466qYlm/ /HWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768943088; x=1769547888; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Ai1otIKDYCwf2Y8TFssFgbpLXbDAkZ98nVLTsrDMfKQ=; b=n5ewMlxKC4XL17sXQjFZREBOOShQd78dVHLZCpqp5yYt6tjpwEEluDdlVSqMKUEgtP Eq7BU9PGa2EhI6sKh0gbe/bU49hyfTKVt+ZFM9uFqYlZd1G7dhIAMLSEnZBCmHLBHu8F DHAl+j5nQ0ZE2v3T0AEnw8HOOxTP9h54tLVHCbuAEwNX36R2JZEwUVL8tpGGsya6AbXH S6pj6cB0SqZhHxTkmontBuq06AOa9KyZfJTcPvQSfbLiVoqPy+chrW6mkQOKhQmAZinS zprJNgYyqLuHHIrDDC2TdxpBzXqp+n7WMguigAdc3Kn/b57cBIyWF9xLYHEIc9tQ3UXf Z3tQ== X-Forwarded-Encrypted: i=1; AJvYcCVjv2SW/pPia1oE8SFWO4PI/ssU7hY/MT18I7XmHMxrVfK0Agsv1axcIIre/cqWOiJIarzSuLx297+VSw0=@vger.kernel.org X-Gm-Message-State: AOJu0Yz2/s1QYzw5vQalVwzrjvNapBqIFFlxPAvY3AQWDYyWKx/KcHcB HymNQNZwOdlvSu/1XvZKayn7mqS7xmoKBamCx/amdnXp265b/K+cbLOv3wm2Q2d8Qw== X-Gm-Gg: AY/fxX7OxiG3UhRwx9H4wzCrezXwpnxMoYlFF3MFyGtqukrAC7UJhBc0KjyPEAeqs+t FWfbTvuHbFOJ+WxZ1cNurz87nUnL+zlCV0KJlG52EXH33n8hoMj71oImAnRdZZdBnEj/Kltl6AY LSP6G6izTWku6vHopjx1cP1Cb6a1RZJvtk8p6SYV8pyz/Z66cteEnxg74jMKL/wdkK+1x+9f14S KN8giAxFY6Rp0YNwWGWWRyU//snxPCh73a9lYA4AMQ6lATwq4ZcrC+plHCBm4VI5WT6p7+kE78e b8j+PtfWjKFpR8C0E50c9dREKSIK0kUgf/8k4R1MOCdlL/BB0WU7cEcEnGStZiWrop+nK4PSZU/ owrdCUdS0wwLr546qy4c/9GZX/YXv1cMrUxG49QdYYeUj0GCR1Wb8y2lxUgEWSys95/3U0Gyyyh y/Ud3wf5PHvnKYthNfkgzo2w== X-Received: by 2002:a05:7022:6183:b0:123:35cb:96e3 with SMTP id a92af1059eb24-1244b37e603mr12727280c88.46.1768943088165; Tue, 20 Jan 2026 13:04:48 -0800 (PST) Received: from ?IPV6:2804:14d:5c54:4efb::1c9d? ([2804:14d:5c54:4efb::1c9d]) by smtp.gmail.com with ESMTPSA id a92af1059eb24-1244ac58140sm19748304c88.4.2026.01.20.13.04.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 20 Jan 2026 13:04:47 -0800 (PST) Message-ID: Date: Tue, 20 Jan 2026 18:04:43 -0300 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: [PATCH 1/2] net/sched: act_gate: fix schedule updates with RCU swap To: Paul Moses , netdev@vger.kernel.org Cc: Jamal Hadi Salim , Cong Wang , Jiri Pirko , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <20260120004720.1886632-1-p@1g4.org> <20260120004720.1886632-2-p@1g4.org> Content-Language: en-US From: Victor Nogueira In-Reply-To: <20260120004720.1886632-2-p@1g4.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 19/01/2026 21:48, Paul Moses wrote: > Switch act_gate parameters to an RCU-protected pointer and update schedule > changes using a prepare-then-swap pattern. This avoids races between the > timer/data paths and configuration updates, and cancels the hrtimer > before swapping schedules. > > A gate action replace could free and swap schedules while the hrtimer > callback or data path still dereferences the old entries, leaving a > use-after-free window during updates. The deferred swap and RCU free > close that window. A reproducer is available on request. > > Also clear params on early error for newly created actions to avoid > leaving a dangling reference. > [...] > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c > index c1f75f2727576..3ee07c3deaf97 100644 > --- a/net/sched/act_gate.c > +++ b/net/sched/act_gate.c > @@ -6,6 +6,7 @@ > #include > #include > #include > +#include Do you really need to include this? > [...] > @@ -69,12 +71,14 @@ static enum hrtimer_restart gate_timer_func(struct hrtimer *timer) > { > struct tcf_gate *gact = container_of(timer, struct tcf_gate, > hitimer); > - struct tcf_gate_params *p = &gact->param; > + struct tcf_gate_params *p; When adding/editing local variables, you should adhere to the reverse xmas tree style [1]. > spin_lock(&gact->tcf_lock); Shouldn't you call rcu_read_lock before this line now? > + p = rcu_dereference_protected(gact->param, > + lockdep_is_held(&gact->tcf_lock)); > [...] > static int tcf_gate_init(struct net *net, struct nlattr *nla, > @@ -296,20 +296,26 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, > struct netlink_ext_ack *extack) > { > struct tc_action_net *tn = net_generic(net, act_gate_ops.net_id); > - enum tk_offsets tk_offset = TK_OFFS_TAI; > - bool bind = flags & TCA_ACT_FLAGS_BIND; > struct nlattr *tb[TCA_GATE_MAX + 1]; > struct tcf_chain *goto_ch = NULL; > - u64 cycletime = 0, basetime = 0; > - struct tcf_gate_params *p; > - s32 clockid = CLOCK_TAI; > + struct tcf_gate_params *p, *oldp; > struct tcf_gate *gact; > struct tc_gate *parm; > - int ret = 0, err; > - u32 gflags = 0; > - s32 prio = -1; > + struct tcf_gate_params newp = { }; Abide by reverse xmas tree when adding local variables. > [...] > + bool clockid_set = false; I could be missing something, but I don't believe you need this boolean. > [...] > @@ -323,6 +329,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla, > > if (tb[TCA_GATE_CLOCKID]) { > clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]); > + clockid_set = true; > switch (clockid) { Instead of using clockid_set and repeating the switch statament. You could put this if-statement after you already have oldp and do the following: if (tb[TCA_GATE_CLOCKID]) { clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]); switch (clockid) { case CLOCK_REALTIME: tk_offset = TK_OFFS_REAL; break; case CLOCK_MONOTONIC: tk_offset = TK_OFFS_MAX; break; case CLOCK_BOOTTIME: tk_offset = TK_OFFS_BOOT; break; case CLOCK_TAI: tk_offset = TK_OFFS_TAI; break; default: NL_SET_ERR_MSG(extack, "Invalid 'clockid'"); return -EINVAL; } } else if (ret != ACT_P_CREATED) { clockid = oldp->tcfg_clockid; tk_offset = gact->tk_offset; } > [...] > - if (tb[TCA_GATE_CYCLE_TIME]) > + if (ret == ACT_P_CREATED) > + update_timer = true; > [...] Here you are assigning update_timer to true when the op is a create... > [...] > + if (update_timer && ret != ACT_P_CREATED) > + hrtimer_cancel(&gact->hitimer); .. however in the if-statement where it is used you are only allowing updates. This looks weird. > [...] > +free_p: > + release_entry_list(&p->entries); > + kfree(p); The 2 lines of code above are being repeated below and in tcf_gate_params_release. You should put them in a common function. > +release_new_entries: > + release_entry_list(&newp.entries); > +put_chain: > if (goto_ch) > tcf_chain_put_by_act(goto_ch); > release_idr: > - /* action is not inserted in any list: it's safe to init hitimer > - * without taking tcf_lock. > - */ > - if (ret == ACT_P_CREATED) > - gate_setup_timer(gact, gact->param.tcfg_basetime, > - gact->tk_offset, gact->param.tcfg_clockid, > - true); > + if (ret == ACT_P_CREATED) { > + p = rcu_dereference_protected(gact->param, 1); > + if (p) { > + release_entry_list(&p->entries); > + kfree(p); > + rcu_assign_pointer(gact->param, NULL); > + } > + } > tcf_idr_release(*a, bind); Also, the AI review [2] pointed out a real issue. It's easy to reproduce by running something like: tc action add action gate base-time 200000000000ns \ sched-entry close 0ns index 10 I think overall you have the right idea - RCU seems like a good fit here. The issue is that this patch is confusing because it seems like you are trying to fix the bug and perform cleanups at the same time. If that is the case, can you try breaking this patch into two? Do one to fix the bug (introducing RCU and etc) and another for the cleanups. [1] https://www.kernel.org/doc/html/v6.3/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs [2] https://netdev-ai.bots.linux.dev/ai-review.html?id=cdc17d0d-fd59-41a8-9c8d-1a42699167fd#patch-0 cheers, Victor