From: Jamal Hadi Salim <[email protected]> Sent: Monday, August 10, 2026 4:00 PM
>Hi, > >On Fri, Aug 7, 2026 at 6:23 AM Jedrzej Jagielski ><[email protected]> wrote: >> >> Fix u32_change() to pass proper variable to u32_replace_hw_knode(). >> >> When trying to offload u32 cmd by adding skip_sw attributes it gets >> ignored and it cannot be correctly processed by HW and tc_cls_u32_offload >> struct which is then passed to driver lacks skip_sw var enabled. >> >> u32_replace_hw_knode is fed with the TCA_ACT_* flags (bits 16+) >> instead of TCA_CLS_* flags (bits 0-4) which actually stores the flags >> parsed by tc, so tc_skip_sw() always returns false. This leads to >> ignoring some of the attibutes which are meant to be configured on filter >> setup. >> >> None of the TCA_ACT_FLAGS_* is actually used within >> u32_replace_hw_knode(), so there is no point in passing them. >> >> Looks like commit 695176bfe5de ("net_sched: refactor TC action init API") >> shadowed the local flags variable which used to be set with >> nla_get_u32(tb[TCA_U32_FLAGS]) with the flags as the new function param >> while not replacing for none of the u32_replace_hw_knode() calls. >> > Hi Jamal >The sashiko reviews look legit to me. Mainly: your change makes >u32_replace_hw_knode() able to fail for the first time (with the >action flags it always returned 0), which turns three previously-dead >error paths into reachable bugs. ohh now i see tc_setup_cb_add() indeed won't make it fail as always returns >= 0 probably would be also worth to clean it in order to not confuse people, wdut? >So IMO, this needs a v2. Will be prepared, sure Thanks for the suggestions! >Suggestions for v2 from the sashiskos (I typically suck at these >summaries, so may miss something): >- Release n->ht_down on the create-path error (e.g. drop the reference >explicitly before kfree(n), or reuse __u32_destroy_key()). >- Consider switching the update path to tc_setup_cb_replace() (as >cls_bpf's cls_bpf_offload_cmd does) so old/new flags and in_hw_count >are accounted for correctly, and the old rule is re-installed on >failure. >- Mention in the commit message that skip_sw add/replace commands that >used to succeed (and were silently skipped by u32_classify()) will now >return -EOPNOTSUPP or -EINVAL when the block/driver cannot offload. > >Also seems worth Cc: stable backport. cc tag already there, isn't it? or do i need to do any extra step? regards, jedrek > >cheers, >jamal > > > > >> Fixes: 695176bfe5de ("net_sched: refactor TC action init API") >> Cc: <[email protected]> >> Reviewed-by: Przemek Kitszel <[email protected]> >> Reviewed-by: Marcin Szycik <[email protected]> >> Signed-off-by: Jedrzej Jagielski <[email protected]>
