On Tue, May 26, 2026 at 9:08 AM David Laight
[off-list ref] wrote:

On Tue, 26 May 2026 07:57:08 -0400
Jamal Hadi Salim [off-list ref] wrote:

quoted

On Tue, May 26, 2026 at 5:48 AM David Laight
[off-list ref] wrote:

quoted

On Sat, 23 May 2026 12:57:08 -0400
Jamal Hadi Salim [off-list ref] wrote:

quoted

On Sat, May 23, 2026 at 12:46 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Sat, 23 May 2026 08:13:21 -0400 Jamal Hadi Salim wrote:

quoted

quoted

quoted

@@ -474,8 +473,6 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
                }

                *ptr = ((*ptr & tkey->mask) ^ val);
-               if (ptr == &hdata)
-                       skb_store_bits(skb, hoffset + offset, ptr, 4);
        }

        goto done;

I see you are trying to get rid of the skb_header_pointer() /
skb_store_bits() piece. Sure looks cleaner if we must linearize.

The other thing (i may be over thinking) with pskb_may_pull is: if the
data is already linear (in a clone), wouldn't we corrupt the shared
linear data of the clone?

I said

  for the portion of the problem that's "we are writing to frags"

IOW not replacing the rest of the patch (assuming we care).

So as an alternative to the piece i posted? i.e this:

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 79921b8d89ba..8f0f84b50c85 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c

@@ -474,6 +474,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
                if (write_offset < 0) {
                        if (skb_cow(skb, -write_offset))
                                goto bad;
+                       if (write_offset + (int)sizeof(hdata) > 0) {
+                               if (skb_ensure_writable(skb,
+                                               min_t(int, skb->len,
+                                                     write_offset +

(int)sizeof(hdata))))

I don't think that needs to be min_t(), a plain min() will do.
(Possibly with the (int) cast removed from the sizeof.)

what is the benefit? would min() allow a mixed type (eg skb_len not being int)?

min_t() is really a broken idea.
You wouldn't really write:
        if ((int)skb->len < (int)(write_offset + (int)sizeof(hdata)))
which it what it effectively expands to.
IMHO it was always better to cast the one side that was 'wrong'.
Then there wouldn't be all the broken/pointless max_t(u8, x, 255).

Here min(skb->len, write_offset + (int)sizeof(hdata)) is actually likely to
be allowed even though they types are unsigned and signed because the
compiler knows the preceding 'if' stops negative values getting through.
(Sometimes the KASAN (etc) builds (randomly) error such cases.)

But min(skb->len, write_offset + sizeof(hdata)) will always be fine.

So i am looking at min(a,b) macro
It does a strict type check to ensure that both arguments have the
same _exact_ type.
But i will take your word for it since you seem to be keeping on top
of these things.
I will update this in the next patch. And if the test passes i will post

cheers,
jamal

Both sides are unsigned, a negative 32bit write_offset is sign extended
to 64bits before being converted to unsigned (as 2s compliment) and the
sum is positive.
But the compiler might not do CSE with the previous condition.

-- David

quoted

cheers,
jamal

quoted

-- David

quoted

+                                       goto bad;
+                       }
                } else {
                        if (unlikely(check_add_overflow(write_offset,
                                                        (int)sizeof(hdata),

cheers,
jamal