Re: [PATCH net v3 2/4] net/sched: act_ife: require a full Ethernet header on egress

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit is titled "require a full Ethernet header on
  egress".
- [Low] The commit message says "A short egress skb is now dropped
  instead of being encoded." The patch actually drops every egress skb…

--

net/sched: act_ife: require a full Ethernet header on egress

On egress, tcf_ife_is_eth_skb() now requires pskb_may_pull(skb, ETH_HLEN)
to succeed. Without that check, ife_encode() could copy ETH_HLEN bytes from
past the linear data of a short skb. The ingress mac_len check is unchanged.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260821164031.32824-1-victor%40mojatatu.com
Link: https://sashiko.dev/#/patchset/20260821164031.32824-1-victor@mojatatu.com
Link: https://lore.kernel.org/netdev/20261022144736.61227-1-shawnq1@163.com/

> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index 6b23a3965121e..5a527da005661 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -878,13 +878,16 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
>   * is an Ethernet header because the skb could've been redirected with mirred
>   * from a non-Ethernet device.
>   */
> -static bool tcf_ife_is_eth_skb(const struct sk_buff *skb)
> +static bool tcf_ife_is_eth_skb(struct sk_buff *skb)
>  {
>  	if (skb->dev->type != ARPHRD_ETHER &&
>  	    skb->dev->type != ARPHRD_LOOPBACK)
>  		return false;
>  
> -	return !skb_at_tc_ingress(skb) || skb->mac_len == ETH_HLEN;
> +	if (skb_at_tc_ingress(skb))
> +		return skb->mac_len == ETH_HLEN;
> +
> +	return pskb_may_pull(skb, ETH_HLEN);

[Severity: Medium]
The subject says "require a full Ethernet header on egress". The commit
message also says tcf_ife_is_eth_skb() "returns true even when the skb has
no Ethernet header".

Does the new egress branch check that the skb has an Ethernet header, or
only that ETH_HLEN bytes are in the linear area?

Take the reproducer setup from the commit message and send an IP packet of
14 bytes or more instead of a short one:

tun_get_user() (IFF_TUN)
    skb_reset_network_header(skb);       /* mac_len is 0 */
tcf_mirred_to_dev()
    mac_len = at_ingress ? skb->mac_len : skb_network_offset(skb);
    skb_push_rcsum(skb_to_send, mac_len); /* pushes 0 bytes */
lo egress: tcf_ife_act()
    tcf_ife_is_eth_skb()
        pskb_may_pull(skb, ETH_HLEN)      /* succeeds */
    tcf_ife_encode()->ife_encode()
        iethh = (struct ethhdr *) skb->data;
        __skb_push(skb, total_push);
        memcpy(skb->data, iethh, ETH_HLEN);

At this point skb->data still points at the IP header. The first 14 IP
header bytes are copied as the inner "original" Ethernet header. They also
end up in the outer h_dest/h_source unless eth_dst/eth_src are configured.

The over-read past the linear data is fixed, and these bytes are packet
data the sender controls, not kernel memory. This behaviour for L3 skbs of
14 bytes or more was already there after commit 5b483f7791b0. Still, a
non-Ethernet frame can be encoded on egress as if it had an Ethernet
header, which is what the subject and issue statement say this patch
prevents.

The comment above the function also still reads as a pure check:

    IFE encapsulates the original Ethernet header and, on decode, expects
    to find one, so it can only ever work on skbs that carry one.

It does not say that the egress branch only checks linear length, or that
the call can now pull and so modify the skb.

Should the subject, issue statement and function comment say that egress
only checks linear length? Or should the egress branch also check that the
L2 header really is Ethernet?

[Severity: Low]
The commit message says "A short egress skb is now dropped instead of
being encoded." Is that the only behaviour change?

tcf_ife_act() calls tcf_ife_is_eth_skb() before choosing between encode
and decode. So a short egress skb is now dropped even when tcf_ife_encode()
would never have encoded it.

ife_get_sz() returns 0 when no metadata is present. One example is
ife_check_meta_u32() with no skb->mark and no configured value. In that
case tcf_ife_encode() returned before touching skb->data:

net/sched/act_ife.c:tcf_ife_encode() {
    ...
	if (!metalen) {		/* no metadata to send */
		...
		qstats_cpu_overlimit_inc(ife->common.cpu_qstats);
		return action;
	}
    ...
}

These sub-ETH_HLEN frames used to pass with the configured action and were
counted as overlimits. Now tcf_ife_act() drops them with TC_ACT_SHOT and
counts them as drops.

Could the commit message mention this change in behaviour and statistics?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com