| Message ID | 20241223133915.2333146-1-antonio.pastor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: llc: explicitly set skb->transport_header | expand |
On Mon, Dec 23, 2024 at 2:40 PM Antonio Pastor <antonio.pastor@gmail.com> wrote: > > 802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA > have skb->transport_header set two bytes short, or pointing 2 bytes > before network_header & skb->data. As snap_rcv expects transport_header > to point to SNAP header (OID:PID) after LLC processing advances offset > over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match > and packet is dropped. > > Between napi_complete_done and snap_rcv, transport_header is not used > until __netif_receive_skb_core, where originally it was being reset. > Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()") > only does so if not set, on the assumption the value was set correctly > by GRO (and also on assumption that "network stacks usually reset the > transport header anyway"). Afterwards it is moved forward by > llc_fixup_skb. > > Locally generated traffic shows up at __netif_receive_skb_core with no > transport_header set and is processed without issue. On a setup with > GRO but no DSA, transport_header and network_header are both set to > point to skb->data which is also correct. > > As issue is LLC specific, to avoid impacting non-LLC traffic, and to > follow up on original assumption made on previous code change, > llc_fixup_skb to reset and advance the offset. llc_fixup_skb already > assumes the LLC header is at skb->data, and by definition SNAP header > immediately follows. > > Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()") > Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com> 11 years with this stuff being broken... I wonder if we could remove it from the kernel, given nobody cared. Can you at the same time fix net/802/psnap.c, snap_rcv() is probably having the same issue ? I would also use skb_reset_transport_header() as in diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c index 51bccfb00a9cd9f16318bbe9a8cc3fe2460912b1..61b0159b2fbee60bdc2623b6c73ed1651b17a050 100644 --- a/net/llc/llc_input.c +++ b/net/llc/llc_input.c @@ -124,8 +124,8 @@ static inline int llc_fixup_skb(struct sk_buff *skb) if (unlikely(!pskb_may_pull(skb, llc_len))) return 0; - skb->transport_header += llc_len; skb_pull(skb, llc_len); + skb_reset_transport_header(skb); if (skb->protocol == htons(ETH_P_802_2)) { __be16 pdulen; s32 data_size;
On Mon, Dec 23, 2024 at 6:00 PM Antonio Pastor <antonio.pastor@gmail.com> wrote: > > On 2024-12-23 08:54, Eric Dumazet wrote: > > On Mon, Dec 23, 2024 at 2:40 PM Antonio Pastor <antonio.pastor@gmail.com> wrote: > > 802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA > have skb->transport_header set two bytes short, or pointing 2 bytes > before network_header & skb->data. As snap_rcv expects transport_header > to point to SNAP header (OID:PID) after LLC processing advances offset > over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match > and packet is dropped. > > Between napi_complete_done and snap_rcv, transport_header is not used > until __netif_receive_skb_core, where originally it was being reset. > Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()") > only does so if not set, on the assumption the value was set correctly > by GRO (and also on assumption that "network stacks usually reset the > transport header anyway"). Afterwards it is moved forward by > llc_fixup_skb. > > Locally generated traffic shows up at __netif_receive_skb_core with no > transport_header set and is processed without issue. On a setup with > GRO but no DSA, transport_header and network_header are both set to > point to skb->data which is also correct. > > As issue is LLC specific, to avoid impacting non-LLC traffic, and to > follow up on original assumption made on previous code change, > llc_fixup_skb to reset and advance the offset. llc_fixup_skb already > assumes the LLC header is at skb->data, and by definition SNAP header > immediately follows. > > Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()") > Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com> > > Thanks for your reply. > > 11 years with this stuff being broken... > > It works great without DSA thrown in the mix, for some reason I have not > been able to determine, not for the lack of trying. Wonder how many > configurations use DSA and care about SNAP frames. > DSA was added recently to the implementation I'm working on, so for > practical purposes this is just broken since 1-2 years ago. Tripped on it > about 6 months ago. It's fresh. Or well aged. :) > > I wonder if we could remove it from the kernel, given nobody cared. > > Well, at least I do, and ballpark 40 other people for what I can tell. > Some others might be lurking in the sidelines. > > Can you at the same time fix net/802/psnap.c, > snap_rcv() is probably having the same issue ? > > That's how I got here, because of snap_rcv(). But no fix is required at > snap_rcv(). This patch is to fix problem there. > snap_rcv() doesn't advance the transport_header offset or pull the skb as > far as I saw so adding a reset there, that would be redundant. Once > this piece of code determines LLC header size SNAP header always follows > for SNAP frames. > I'll double-check and submit a v3 w/that included if I think it makes > sense but honestly I don't think it does. I see skb->transport_header being advanced at line 61 : /* Pass the frame on. */ skb->transport_header += 5; Note that setting the transport header (conditionally or not) in __netif_receive_skb() is probably a mistake. Only network (ipv4, ipv6) handlers can possibly know the concept of transport header. Hopefully at some point we can remove this defensive code. diff --git a/net/core/dev.c b/net/core/dev.c index c7f3dea3e0eb9eb05865e49dd7a8535afb974149..b6722e11ee4e171e6a2f379fc1d0197dfcb1a842 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5474,8 +5474,6 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc, orig_dev = skb->dev; skb_reset_network_header(skb); - if (!skb_transport_header_was_set(skb)) - skb_reset_transport_header(skb); skb_reset_mac_len(skb); pt_prev = NULL;
diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c index 51bccfb00a9c..6f33ae9095f8 100644 --- a/net/llc/llc_input.c +++ b/net/llc/llc_input.c @@ -124,7 +124,7 @@ static inline int llc_fixup_skb(struct sk_buff *skb) if (unlikely(!pskb_may_pull(skb, llc_len))) return 0; - skb->transport_header += llc_len; + skb_set_transport_header(skb, llc_len); skb_pull(skb, llc_len); if (skb->protocol == htons(ETH_P_802_2)) { __be16 pdulen;
802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA have skb->transport_header set two bytes short, or pointing 2 bytes before network_header & skb->data. As snap_rcv expects transport_header to point to SNAP header (OID:PID) after LLC processing advances offset over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match and packet is dropped. Between napi_complete_done and snap_rcv, transport_header is not used until __netif_receive_skb_core, where originally it was being reset. Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()") only does so if not set, on the assumption the value was set correctly by GRO (and also on assumption that "network stacks usually reset the transport header anyway"). Afterwards it is moved forward by llc_fixup_skb. Locally generated traffic shows up at __netif_receive_skb_core with no transport_header set and is processed without issue. On a setup with GRO but no DSA, transport_header and network_header are both set to point to skb->data which is also correct. As issue is LLC specific, to avoid impacting non-LLC traffic, and to follow up on original assumption made on previous code change, llc_fixup_skb to reset and advance the offset. llc_fixup_skb already assumes the LLC header is at skb->data, and by definition SNAP header immediately follows. Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()") Signed-off-by: Antonio Pastor <antonio.pastor@gmail.com> --- net/llc/llc_input.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)