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

[Alexandra Winter]

On 21.08.26 16:55, Hidayath Khan wrote:
> 
> 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

Thank you for your explanations, Bryam.
I agree it makes sense to treat this as a fix.

Reviewed-by: Alexandra Winter <wintera@xxxxxxxxxxxxx>