Re: [PATCH net 2/2] net/iucv: take a private, writable frame before rewriting it in place

On 21/08/26 5:12 pm, Bryam Vargas wrote:
> Alexandra,
>
>> Excuse my ignorance, if it is obvious to other readers, but is the worst
>> thing that the output of tcpdump is not correct?
> Not obvious, and my description is why: it led with tcpdump, which is the
> mildest end of this.
>
> The order is the other way round. __netif_receive_skb_core() walks
> ptype_base[] at net/core/dev.c:6160, before net->ptype_specific (:6169)
> and orig_dev->ptype_specific (:6173). iucv_packet_type sets no .dev and no
> .af_packet_net, so it sits in ptype_base[] while a packet socket for
> ETH_P_AF_IUCV lands in one of the later lists. af_iucv runs first, and the
> AF_PACKET reader gets the frame after EBCASC() has rewritten the four name
> fields. The capture is wrong, but it was already wrong before the reader
> was reached.
>
> That isn't what I'd defend the patch on. Because af_iucv isn't the last
> matching handler in that configuration, deliver_ptype_list_skb() hands it
> over through deliver_skb(), which does refcount_inc(&skb->users) before
> calling us (dev.c:2492, :2507). We run with users == 2, and on that skb we
> rewrite the header in place, skb_push() 14 bytes in afiucv_swap_src_dest()
> and pass the same skb to dev_queue_xmit() (af_iucv.c:1876, :1888, :1914) --
> including for a frame that matched no socket (:1872).
>
> What hides it in review is a guard asymmetry. deliver_skb() leaves
> users == 2 with skb->cloned == 0, so skb_shared() is true while
> skb_cloned() is false, and the copy-on-write guards all test skb_cloned()
> -- __pskb_pull_tail() at skbuff.c:2886 among them -- so they read the skb
> as already writable. The one that does test it is BUG_ON(skb_shared(skb))
> at the top of pskb_expand_head() (skbuff.c:2305); skb_expand_head() carries
> "/* pskb_expand_head() might crash, if skb is shared. */" (:2456) for the
> same reason.
>
> What I don't have is a panic. On the qeth geometry the first
> pskb_may_pull() finds enough tailroom in the napi_get_frags() head and
> copies out of the frags without expanding, so it doesn't reach
> pskb_expand_head that way. By inspection; not reproduced.
>
>> Is this really a problem fix then? Or should it go to net-next?
> If the bar is a failure I can show you, net-next is right. I sent it to net
> because a handler that writes a shared skb and then gives it to the
> transmit path is a rule violation with a BUG_ON behind it, not because I
> can fire that BUG_ON. Your call either way, and net-next is fine by me.
>
> Worth having in the record: reaching the shared state costs one syscall --
> socket(AF_PACKET, SOCK_RAW, htons(0xFBFB)), no bind, no ETH_P_ALL -- since
> ptype_base[] is walked before the per-namespace list.
>
> If Hidayath's version is further along, take his. I'd rather the check land
> than land mine.
Hi Bryam,

Please go ahead with your patch. I had dropped my patch and am not 
pursuing it.

Thanks,
Hidayath
>
> Thanks,
> Bryam
>