Re: [PATCH] bridge: add ETH_HLEN to packet_length
On Thu, 14 Sep 2006 13:27:54 -0500 Jon Mason <[EMAIL PROTECTED]> wrote: > In br_dev_queue_push_xmit, why is the check to drop mtu oversized > packets not checking for enough room for the impending ETH_HLEN size > skb_push? In some code currently under development, we are seeing > skb_under_panic being called from the "skb_push(skb, ETH_HLEN)" in that > code. It seems to me it would be better to drop those skbs than panic. > Attached is a patch to do this. > > Thanks, > Jon This is bridge code, it assumes that any device that claims to be a ethernet device has done the proper reservation. What device are you using? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bridge: add ETH_HLEN to packet_length
On Thu, Sep 14, 2006 at 11:53:48PM +0300, Mika Penttil? wrote: > Jon Mason wrote: > >In br_dev_queue_push_xmit, why is the check to drop mtu oversized > >packets not checking for enough room for the impending ETH_HLEN size > >skb_push? In some code currently under development, we are seeing > >skb_under_panic being called from the "skb_push(skb, ETH_HLEN)" in that > >code. It seems to me it would be better to drop those skbs than panic. > >Attached is a patch to do this. > > > >Thanks, > >Jon > > > >Signed-off-by: Jon Mason <[EMAIL PROTECTED]> > > > >diff -r b1d36669f98d net/bridge/br_forward.c > >--- a/net/bridge/br_forward.cMon Sep 4 03:00:04 2006 + > >+++ b/net/bridge/br_forward.cThu Sep 14 13:18:04 2006 -0500 > >@@ -29,7 +29,8 @@ static inline int should_deliver(const s > > > > static inline unsigned packet_length(const struct sk_buff *skb) > > { > >-return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : > >0); > >+return skb->len - ETH_HLEN - > >+(skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); > > } > > > > > packet_length() is a wrong place to do that, mtu has nothing to do with > skb headroom. Oops, you are 100% correct. So what would be the best way to handle this, as a panic is pretty nasty? Wouldn't it be better to check the packet to see if there is sufficient headroom, BUG, and then drop it (or allocate a new packet with sufficient headroom and proceed with the skb_push)? Thanks, Jon > > --Mika > > - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bridge: add ETH_HLEN to packet_length
Jon Mason wrote: In br_dev_queue_push_xmit, why is the check to drop mtu oversized packets not checking for enough room for the impending ETH_HLEN size skb_push? In some code currently under development, we are seeing skb_under_panic being called from the "skb_push(skb, ETH_HLEN)" in that code. It seems to me it would be better to drop those skbs than panic. Attached is a patch to do this. Thanks, Jon Signed-off-by: Jon Mason <[EMAIL PROTECTED]> diff -r b1d36669f98d net/bridge/br_forward.c --- a/net/bridge/br_forward.c Mon Sep 4 03:00:04 2006 + +++ b/net/bridge/br_forward.c Thu Sep 14 13:18:04 2006 -0500 @@ -29,7 +29,8 @@ static inline int should_deliver(const s static inline unsigned packet_length(const struct sk_buff *skb) { - return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); + return skb->len - ETH_HLEN - + (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); } packet_length() is a wrong place to do that, mtu has nothing to do with skb headroom. --Mika - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bridge: add ETH_HLEN to packet_length
In br_dev_queue_push_xmit, why is the check to drop mtu oversized packets not checking for enough room for the impending ETH_HLEN size skb_push? In some code currently under development, we are seeing skb_under_panic being called from the "skb_push(skb, ETH_HLEN)" in that code. It seems to me it would be better to drop those skbs than panic. Attached is a patch to do this. Thanks, Jon Signed-off-by: Jon Mason <[EMAIL PROTECTED]> diff -r b1d36669f98d net/bridge/br_forward.c --- a/net/bridge/br_forward.c Mon Sep 4 03:00:04 2006 + +++ b/net/bridge/br_forward.c Thu Sep 14 13:18:04 2006 -0500 @@ -29,7 +29,8 @@ static inline int should_deliver(const s static inline unsigned packet_length(const struct sk_buff *skb) { - return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); + return skb->len - ETH_HLEN - + (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); } int br_dev_queue_push_xmit(struct sk_buff *skb) - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html