* Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon
From: Eugene Krasnikov @ 2013-11-07 7:32 UTC (permalink / raw)
To: Joe Perches
Cc: Luis R. Rodriguez, John W. Linville, wcn36xx, linux-wireless,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
ath5k-devel-xDcbHBWguxEUs3QNXV6qNA, ath9k-devel,
ath10k-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1383760556.7940.28.camel@joe-AO722>
Hi Joe,
I personally like the idea of making some kind of framework on top of
printing because all ath drivers are using the same printing approach
and combining all that code in one place will reduce amount of code in
each particular driver. As a true engineer i like when it's less code
= less work to do = less bugs:)
Suggestion is to send a patch with semicolon fix only and have a
second round of convincing ath guys to change printing code. How does
that sound?
On Wed, Nov 6, 2013 at 5:55 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Wed, 2013-11-06 at 07:49 +0000, Eugene Krasnikov wrote:
>> Hm... when it comes to semicolon the patch seems to be good. When it
>> comes to dynamic debugging i think we should have a separate
>> discussion about that.
>> I personally like the whole idea about dynamic debug but if you want
>> to change it i would suggest to have some kind of framework for all
>> ath drivers(or maybe all wireless drivers). Because obviously you can
>> find common code in every driver that defines it's own debug
>> functions/debug levels and so on. Why not to make a framework with
>> standard API/levels?
>
> You need to bring that up with the Atheros folk.
> I've tried. The view seemed to be it was more
> trouble than it was worth.
>
>
>
--
Best regards,
Eugene
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 7:31 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107071531.GA31857@gondor.apana.org.au>
On Thu, 2013-11-07 at 15:15 +0800, Herbert Xu wrote:
> So what in our stack violates this assumption? We've never handled
> arbitrary frag_lists in GSO and I see no reason why we need to start
> doing that now.
I do see this, skb_segment() is generic.
>
> Also GRO was designed to only merge packets that satisfy these
> assumptions so that through GSO the original packets can be
> recovered without losing end-to-end connectivity. This is really
> important for routers/switches.
I think we all agree on this, and we should keep this property.
The point is : skb_segment() is not tied to GRO anymore.
My patch handles virtio_net just fine, I see nothing really malicious in
virtio_net.
In particular, each skb found in the frag_list can be of any size,
and not an exact MSS multiple.
I see frag_list as a way to extend skb capacity, not as something
tied to GRO/GSO.
I worked last year so that we no longer had the frag_list being used in
GRO stack. frag_list was no longer needed, thanks to skb->head_frag
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Herbert Xu @ 2013-11-07 7:17 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107071531.GA31857@gondor.apana.org.au>
On Thu, Nov 07, 2013 at 03:15:31PM +0800, Herbert Xu wrote:
> On Wed, Nov 06, 2013 at 11:11:02PM -0800, Eric Dumazet wrote:
> > On Thu, 2013-11-07 at 08:43 +0800, Herbert Xu wrote:
> >
> > > - if (fskb != skb_shinfo(skb)->frag_list)
> > > - goto perform_csum_check;
> > > + if (fskb != skb_shinfo(skb)->frag_list) {
> > > + struct sk_buff *nsegs;
> > > +
> > > + if (nskb->len == len + doffset)
> > > + goto perform_csum_check;
> > > +
> > > + if (skb_has_frag_list(nskb)) {
> > > + net_warn_ratelimited(
> > > + "skb_segment: "
> > > + "nested frag_list detected\n");
> > > + kfree(nskb);
> > > + err = -EINVAL;
> > > + goto err;
> > > + }
> > > +
> > > + __skb_pull(nskb, doffset);
> > > + skb_shinfo(nskb)->gso_size = mss;
> > > + nsegs = skb_segment(nskb, features);
> > > +
> >
> > This still assumes each skb found in frag_list has a exact multiple of
> > @mss bytes, and that the initial skb also ends at a right boundary.
>
> So what in our stack violates this assumption? We've never handled
> arbitrary frag_lists in GSO and I see no reason why we need to start
> doing that now.
>
> Also GRO was designed to only merge packets that satisfy these
> assumptions so that through GSO the original packets can be
> recovered without losing end-to-end connectivity. This is really
> important for routers/switches.
Or perhaps you are saying that this doesn't handle page frags?
It should because each GRO frag_list entry looks just like any
boring old GSO packet filled with frags.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Herbert Xu @ 2013-11-07 7:15 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <1383808262.9412.33.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 11:11:02PM -0800, Eric Dumazet wrote:
> On Thu, 2013-11-07 at 08:43 +0800, Herbert Xu wrote:
>
> > - if (fskb != skb_shinfo(skb)->frag_list)
> > - goto perform_csum_check;
> > + if (fskb != skb_shinfo(skb)->frag_list) {
> > + struct sk_buff *nsegs;
> > +
> > + if (nskb->len == len + doffset)
> > + goto perform_csum_check;
> > +
> > + if (skb_has_frag_list(nskb)) {
> > + net_warn_ratelimited(
> > + "skb_segment: "
> > + "nested frag_list detected\n");
> > + kfree(nskb);
> > + err = -EINVAL;
> > + goto err;
> > + }
> > +
> > + __skb_pull(nskb, doffset);
> > + skb_shinfo(nskb)->gso_size = mss;
> > + nsegs = skb_segment(nskb, features);
> > +
>
> This still assumes each skb found in frag_list has a exact multiple of
> @mss bytes, and that the initial skb also ends at a right boundary.
So what in our stack violates this assumption? We've never handled
arbitrary frag_lists in GSO and I see no reason why we need to start
doing that now.
Also GRO was designed to only merge packets that satisfy these
assumptions so that through GSO the original packets can be
recovered without losing end-to-end connectivity. This is really
important for routers/switches.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] smsc9420: Use netif_<level>
From: David Miller @ 2013-11-07 7:15 UTC (permalink / raw)
To: joe; +Cc: netdev, steve.glendinning, linux-kernel
In-Reply-To: <1383676461.4387.28.camel@joe-AO722>
From: Joe Perches <joe@perches.com>
Date: Tue, 05 Nov 2013 10:34:21 -0800
> Use a more standard logging style.
>
> Convert smsc_<level> macros to use netif_<level>.
> Remove unused #define PFX
> Add pr_fmt and neaten pr_<level> uses.
>
> Signed-off-by: Joe Perches <joe@perches.com>
Also applied, thanks Joe.
^ permalink raw reply
* Re: [PATCH] jme: Remove unused #define PFX
From: David Miller @ 2013-11-07 7:14 UTC (permalink / raw)
To: joe; +Cc: netdev, cooldavid, linux-kernel
In-Reply-To: <1383672595.4387.23.camel@joe-AO722>
From: Joe Perches <joe@perches.com>
Date: Tue, 05 Nov 2013 09:29:55 -0800
> It's unused, remove it.
>
> Signed-off-by: Joe Perches <joe@perches.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next trivial] udp: Remove unnecessary semicolon from do{}while (0) macro
From: David Miller @ 2013-11-07 7:14 UTC (permalink / raw)
To: joe; +Cc: netdev, linux-kernel
In-Reply-To: <1383689627.28624.3.camel@joe-AO722>
From: Joe Perches <joe@perches.com>
Date: Tue, 05 Nov 2013 14:13:47 -0800
> Just an unnecessary semicolon that should be removed...
>
> Whitespace neatening of macro too.
>
> Signed-off-by: Joe Perches <joe@perches.com>
Applied.
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 7:11 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107004339.GA28156@gondor.apana.org.au>
On Thu, 2013-11-07 at 08:43 +0800, Herbert Xu wrote:
> - if (fskb != skb_shinfo(skb)->frag_list)
> - goto perform_csum_check;
> + if (fskb != skb_shinfo(skb)->frag_list) {
> + struct sk_buff *nsegs;
> +
> + if (nskb->len == len + doffset)
> + goto perform_csum_check;
> +
> + if (skb_has_frag_list(nskb)) {
> + net_warn_ratelimited(
> + "skb_segment: "
> + "nested frag_list detected\n");
> + kfree(nskb);
> + err = -EINVAL;
> + goto err;
> + }
> +
> + __skb_pull(nskb, doffset);
> + skb_shinfo(nskb)->gso_size = mss;
> + nsegs = skb_segment(nskb, features);
> +
This still assumes each skb found in frag_list has a exact multiple of
@mss bytes, and that the initial skb also ends at a right boundary.
^ permalink raw reply
* [3/3] gso: Handle malicious GRO packets without crashing
From: Herbert Xu @ 2013-11-07 7:08 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107070647.GB31638@gondor.apana.org.au>
As virtio_net can now generate GRO frag_list packets without
sufficient verification, we need to handle malicious GRO packets
thrown at us.
This patch converts to affected BUG_ONs in skb_segment to rate-
limited warnings.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index bcc3f1c..fb1106d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2881,7 +2881,15 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
while (tail->next)
tail = tail->next;
- BUG_ON(fskb && tail->len != len + doffset);
+ if (fskb && tail->len != len + doffset) {
+ net_warn_ratelimited(
+ "skb_segment: "
+ "illegal GSO fragment: %u %u\n",
+ tail->len, len + doffset);
+ kfree(nskb);
+ err = -EINVAL;
+ goto err;
+ }
len = nskb->len;
kfree(nskb);
@@ -2929,7 +2937,15 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
if (pos < offset + len) {
struct sk_buff *fskb2 = fskb;
- BUG_ON(pos + fskb->len != offset + len);
+ if (pos + fskb->len != offset + len) {
+ net_warn_ratelimited(
+ "skb_segment: "
+ "illegal GSO trailer: %u %u\n",
+ pos + fskb->len, offset + len);
+ kfree(nskb);
+ err = -EINVAL;
+ goto err;
+ }
pos += fskb->len;
fskb = fskb->next;
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 7:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton, Jason Wang
In-Reply-To: <20131107055633.GA28986@redhat.com>
On Thu, 2013-11-07 at 07:56 +0200, Michael S. Tsirkin wrote:
> I considered doing this but won't this mean packets can get reordered?
> In practice we need to maintain ordering of RX frames within
> a given flow, correct?
Sorry, I was referring to two pools of frags, instead of a single one.
One pool of frags of (1500 + hdr) bytes
One pool of frags of 4096 bytes
But its still one logical queue.
^ permalink raw reply
* [2/3] gso: Handle new frag_list of frags GRO packets
From: Herbert Xu @ 2013-11-07 7:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107070335.GA31638@gondor.apana.org.au>
Recently GRO started generating packets with frag_lists of frags.
This was not handled by GSO, thus leading to a crash.
Thankfully these packets are of a regular form and are easy to
handle. This patch handles them by calling skb_segment for each
frag_list entry. The depth of recursion is limited to just one.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 88b7dc6..bcc3f1c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2816,8 +2816,6 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
hsize = len;
if (!hsize && i >= nfrags) {
- BUG_ON(fskb->len != len);
-
pos += len;
nskb = skb_clone(fskb, GFP_ATOMIC);
fskb = fskb->next;
@@ -2855,8 +2853,40 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
nskb->data - tnl_hlen,
doffset + tnl_hlen);
- if (fskb != skb_shinfo(skb)->frag_list)
- goto perform_csum_check;
+ if (fskb != skb_shinfo(skb)->frag_list) {
+ struct sk_buff *nsegs;
+
+ if (nskb->len == len + doffset)
+ goto perform_csum_check;
+
+ SKB_FRAG_ASSERT(nskb);
+
+ __skb_pull(nskb, doffset);
+ skb_shinfo(nskb)->gso_size = mss;
+ nsegs = skb_segment(nskb, features);
+
+ err = PTR_ERR(nsegs);
+ if (IS_ERR(nsegs)) {
+ kfree(nskb);
+ goto err;
+ }
+ err = -ENOMEM;
+
+ if (segs)
+ tail->next = nsegs;
+ else
+ segs = nsegs;
+
+ tail = nsegs;
+ while (tail->next)
+ tail = tail->next;
+
+ BUG_ON(fskb && tail->len != len + doffset);
+
+ len = nskb->len;
+ kfree(nskb);
+ continue;
+ }
if (!sg) {
nskb->ip_summed = CHECKSUM_NONE;
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* [1/3] gso: Add to segs at end of loop in skb_segment
From: Herbert Xu @ 2013-11-07 7:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107062234.GA31156@gondor.apana.org.au>
This patch moves the addition to the segs list to the end of the
loop in skb_segment. This is to allow the following patch to add
a list to segs without including nskb.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3735fad..88b7dc6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2846,12 +2846,6 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
__skb_put(nskb, doffset);
}
- if (segs)
- tail->next = nskb;
- else
- segs = nskb;
- tail = nskb;
-
__copy_skb_header(nskb, skb);
nskb->mac_len = skb->mac_len;
@@ -2869,7 +2863,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
nskb->csum = skb_copy_and_csum_bits(skb, offset,
skb_put(nskb, len),
len, 0);
- continue;
+ goto add_to_segs;
}
frag = skb_shinfo(nskb)->frags;
@@ -2912,8 +2906,10 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
if (fskb2->next) {
fskb2 = skb_clone(fskb2, GFP_ATOMIC);
- if (!fskb2)
+ if (!fskb2) {
+ kfree(nskb);
goto err;
+ }
} else
skb_get(fskb2);
@@ -2932,6 +2928,13 @@ perform_csum_check:
nskb->len - doffset, 0);
nskb->ip_summed = CHECKSUM_NONE;
}
+
+add_to_segs:
+ if (segs)
+ tail->next = nskb;
+ else
+ segs = nskb;
+ tail = nskb;
} while ((offset += len) < skb->len);
return segs;
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 6:59 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton
In-Reply-To: <1383793512.9412.15.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, 2013-11-06 at 19:05 -0800, Eric Dumazet wrote:
> On Thu, 2013-11-07 at 03:03 +0100, Hannes Frederic Sowa wrote:
>
> > > Oh yes, I missed a : data_len += remain;
> > >
> > > at line 2906 :
> > >
> > > offset = remain;
> > > + data_len += remain;
> > > continue;
> >
> > Hm, I still hit the WARN_ON_ONCE (same test as above) with this fixed up:
> >
>
> Thanks, I'll track this and send a v2
>
Here is a new version before I turn off my laptop for the night.
It survived my tests and makes the code cleaner.
Thanks !
net/core/skbuff.c | 190 +++++++++++++++++++++-----------------------
1 file changed, 93 insertions(+), 97 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3735fad..eccd434 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2771,80 +2771,57 @@ EXPORT_SYMBOL_GPL(skb_pull_rcsum);
* a pointer to the first in a list of new skbs for the segments.
* In case of error it returns ERR_PTR(err).
*/
-struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
+struct sk_buff *skb_segment(struct sk_buff *head_skb, netdev_features_t features)
{
struct sk_buff *segs = NULL;
struct sk_buff *tail = NULL;
- struct sk_buff *fskb = skb_shinfo(skb)->frag_list;
- unsigned int mss = skb_shinfo(skb)->gso_size;
- unsigned int doffset = skb->data - skb_mac_header(skb);
- unsigned int offset = doffset;
- unsigned int tnl_hlen = skb_tnl_header_len(skb);
+ struct sk_buff *cskb = head_skb;
+ unsigned int mss = skb_shinfo(head_skb)->gso_size;
+ unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
+ unsigned int tot_len; /* should reach head_skb->len at the end */
+ unsigned int offset = doffset; /* offset in cskb->data */
+ unsigned int tnl_hlen = skb_tnl_header_len(head_skb);
unsigned int headroom;
unsigned int len;
__be16 proto;
bool csum;
int sg = !!(features & NETIF_F_SG);
- int nfrags = skb_shinfo(skb)->nr_frags;
+ int cur_frag = 0, nfrags = skb_shinfo(cskb)->nr_frags;
+ unsigned int data_len, cur_frag_offset = 0;
int err = -ENOMEM;
- int i = 0;
- int pos;
- proto = skb_network_protocol(skb);
+ proto = skb_network_protocol(head_skb);
if (unlikely(!proto))
return ERR_PTR(-EINVAL);
csum = !!can_checksum_protocol(features, proto);
- __skb_push(skb, doffset);
- headroom = skb_headroom(skb);
- pos = skb_headlen(skb);
+ __skb_push(head_skb, doffset);
+ headroom = skb_headroom(head_skb);
- do {
+ for (tot_len = doffset; tot_len < head_skb->len; tot_len += len) {
struct sk_buff *nskb;
skb_frag_t *frag;
- int hsize;
- int size;
+ int hsize, size, remain;
- len = skb->len - offset;
+ len = head_skb->len - tot_len;
if (len > mss)
len = mss;
- hsize = skb_headlen(skb) - offset;
+ hsize = skb_headlen(cskb) - offset;
if (hsize < 0)
hsize = 0;
if (hsize > len || !sg)
hsize = len;
- if (!hsize && i >= nfrags) {
- BUG_ON(fskb->len != len);
-
- pos += len;
- nskb = skb_clone(fskb, GFP_ATOMIC);
- fskb = fskb->next;
+ nskb = __alloc_skb(hsize + doffset + headroom,
+ GFP_ATOMIC, skb_alloc_rx_flag(head_skb),
+ NUMA_NO_NODE);
- if (unlikely(!nskb))
- goto err;
-
- hsize = skb_end_offset(nskb);
- if (skb_cow_head(nskb, doffset + headroom)) {
- kfree_skb(nskb);
- goto err;
- }
+ if (unlikely(!nskb))
+ goto err;
- nskb->truesize += skb_end_offset(nskb) - hsize;
- skb_release_head_state(nskb);
- __skb_push(nskb, doffset);
- } else {
- nskb = __alloc_skb(hsize + doffset + headroom,
- GFP_ATOMIC, skb_alloc_rx_flag(skb),
- NUMA_NO_NODE);
-
- if (unlikely(!nskb))
- goto err;
-
- skb_reserve(nskb, headroom);
- __skb_put(nskb, doffset);
- }
+ skb_reserve(nskb, headroom);
+ __skb_put(nskb, doffset);
if (segs)
tail->next = nskb;
@@ -2852,94 +2829,113 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
segs = nskb;
tail = nskb;
- __copy_skb_header(nskb, skb);
- nskb->mac_len = skb->mac_len;
+ __copy_skb_header(nskb, head_skb);
+ nskb->mac_len = head_skb->mac_len;
skb_headers_offset_update(nskb, skb_headroom(nskb) - headroom);
- skb_copy_from_linear_data_offset(skb, -tnl_hlen,
+ skb_copy_from_linear_data_offset(head_skb, -tnl_hlen,
nskb->data - tnl_hlen,
doffset + tnl_hlen);
- if (fskb != skb_shinfo(skb)->frag_list)
- goto perform_csum_check;
-
if (!sg) {
nskb->ip_summed = CHECKSUM_NONE;
- nskb->csum = skb_copy_and_csum_bits(skb, offset,
+ nskb->csum = skb_copy_and_csum_bits(head_skb, tot_len,
skb_put(nskb, len),
len, 0);
+ offset += len;
continue;
}
frag = skb_shinfo(nskb)->frags;
- skb_copy_from_linear_data_offset(skb, offset,
+ skb_copy_from_linear_data_offset(cskb, offset,
skb_put(nskb, hsize), hsize);
+ offset += hsize;
- skb_shinfo(nskb)->tx_flags = skb_shinfo(skb)->tx_flags & SKBTX_SHARED_FRAG;
-
- while (pos < offset + len && i < nfrags) {
- *frag = skb_shinfo(skb)->frags[i];
- __skb_frag_ref(frag);
- size = skb_frag_size(frag);
+ nskb->data_len = len - hsize;
+ nskb->len += nskb->data_len;
+ nskb->truesize += nskb->data_len;
- if (pos < offset) {
- frag->page_offset += offset - pos;
- skb_frag_size_sub(frag, offset - pos);
+ skb_shinfo(nskb)->tx_flags = skb_shinfo(head_skb)->tx_flags & SKBTX_SHARED_FRAG;
+
+ for (data_len = 0; data_len < nskb->data_len; data_len += remain) {
+ remain = nskb->data_len - data_len;
+ if (unlikely(cur_frag >= nfrags)) {
+ if (cskb == head_skb)
+ cskb = skb_shinfo(head_skb)->frag_list;
+ else
+ cskb = cskb->next;
+ if (!cskb) {
+ WARN_ON_ONCE(1);
+ goto err;
+ }
+ cur_frag = 0;
+ cur_frag_offset = 0;
+ nfrags = skb_shinfo(cskb)->nr_frags;
+ offset = 0;
+ if (skb_headlen(cskb)) {
+ char *data;
+ struct page *page;
+
+ remain = min_t(int, remain, skb_headlen(cskb));
+
+ if (likely(cskb->head_frag)) {
+ data = cskb->data;
+ page = virt_to_head_page(data);
+ get_page(page);
+ } else {
+ data = __netdev_alloc_frag(SKB_DATA_ALIGN(remain),
+ GFP_ATOMIC);
+ /* Really this should not happen, fix the caller ! */
+ WARN_ON_ONCE(1);
+ if (!data)
+ goto err;
+ memcpy(data, cskb->data, remain);
+ page = virt_to_head_page(data);
+ }
+ frag->page.p = page;
+ frag->page_offset = data - (char *)page_address(page);
+ skb_frag_size_set(frag, remain);
+ frag++;
+ offset = remain;
+ continue;
+ }
}
+ *frag = skb_shinfo(cskb)->frags[cur_frag];
+ __skb_frag_ref(frag);
- skb_shinfo(nskb)->nr_frags++;
+ frag->page_offset += cur_frag_offset;
+ skb_frag_size_sub(frag, cur_frag_offset);
+ size = skb_frag_size(frag);
- if (pos + size <= offset + len) {
- i++;
- pos += size;
+ if (size <= remain) {
+ cur_frag++;
+ cur_frag_offset = 0;
+ remain = size;
} else {
- skb_frag_size_sub(frag, pos + size - (offset + len));
- goto skip_fraglist;
+ skb_frag_size_set(frag, remain);
+ cur_frag_offset += remain;
}
frag++;
}
- if (pos < offset + len) {
- struct sk_buff *fskb2 = fskb;
-
- BUG_ON(pos + fskb->len != offset + len);
-
- pos += fskb->len;
- fskb = fskb->next;
-
- if (fskb2->next) {
- fskb2 = skb_clone(fskb2, GFP_ATOMIC);
- if (!fskb2)
- goto err;
- } else
- skb_get(fskb2);
-
- SKB_FRAG_ASSERT(nskb);
- skb_shinfo(nskb)->frag_list = fskb2;
- }
-
-skip_fraglist:
- nskb->data_len = len - hsize;
- nskb->len += nskb->data_len;
- nskb->truesize += nskb->data_len;
+ skb_shinfo(nskb)->nr_frags = frag - skb_shinfo(nskb)->frags;
-perform_csum_check:
if (!csum) {
nskb->csum = skb_checksum(nskb, doffset,
nskb->len - doffset, 0);
nskb->ip_summed = CHECKSUM_NONE;
}
- } while ((offset += len) < skb->len);
+ }
return segs;
err:
- while ((skb = segs)) {
- segs = skb->next;
- kfree_skb(skb);
+ while ((cskb = segs)) {
+ segs = cskb->next;
+ kfree_skb(cskb);
}
return ERR_PTR(err);
}
^ permalink raw reply related
* Re: gso: Attempt to handle mega-GRO packets
From: Herbert Xu @ 2013-11-07 6:22 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107004339.GA28156@gondor.apana.org.au>
On Thu, Nov 07, 2013 at 08:43:39AM +0800, Herbert Xu wrote:
>
> Yeah I screwed up with the test, it needs an additional doffset
> since the tail already has the headers pushed:
OK the patch has passed my tests and I will clean it up for
submission.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Michael S. Tsirkin @ 2013-11-07 5:56 UTC (permalink / raw)
To: Eric Dumazet
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton, Jason Wang
In-Reply-To: <1383789758.2878.32.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 06:02:38PM -0800, Eric Dumazet wrote:
> On Thu, 2013-11-07 at 09:47 +0800, Herbert Xu wrote:
>
> > Say the system is fragmented sufficiently that you'll end up with
> > 0-order pages. In that case you'll only ever be able to coalesce
> > two packets.
>
> 4K page will contain 2 frags and they will coalesce.
>
> Performance will still be quite good.
>
> We probably add a tweak, to not have any hole in this case.
>
> >
> > Real systems that run for more than a day do end up with seriously
> > fragmented memory.
>
> Sure, but having shallow skbs in the first place help quite a bit.
>
> There is no perfect solution, unless of course you change virtio_net to
> provide different queues, with different frag sizes.
>
> Sort of what NIU driver uses.
>
I considered doing this but won't this mean packets can get reordered?
In practice we need to maintain ordering of RX frames within
a given flow, correct?
--
MST
^ permalink raw reply
* [PATCH] netfilter: fix connlimit Kconfig prompt string
From: Randy Dunlap @ 2013-11-07 5:48 UTC (permalink / raw)
To: netdev@vger.kernel.org; +Cc: David Miller, netfilter-devel, lailavrazda1979
From: Randy Dunlap <rdunlap@infradead.org>
Under Core Netfilter Configuration, connlimit match support has
an extra double quote at the end of it.
Fixes a portion of kernel bugzilla #52671:
https://bugzilla.kernel.org/show_bug.cgi?id=52671
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: lailavrazda1979@gmail.com
---
net/netfilter/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- lnx-312.orig/net/netfilter/Kconfig
+++ lnx-312/net/netfilter/Kconfig
@@ -857,7 +857,7 @@ config NETFILTER_XT_MATCH_CONNLABEL
connection simultaneously.
config NETFILTER_XT_MATCH_CONNLIMIT
- tristate '"connlimit" match support"'
+ tristate '"connlimit" match support'
depends on NF_CONNTRACK
depends on NETFILTER_ADVANCED
---help---
^ permalink raw reply
* ??
From: jjorge @ 2013-11-07 4:57 UTC (permalink / raw)
To: Recipients
is it safe to discuss with you in this email?
^ permalink raw reply
* [PATCH] net: x86: bpf: don't forget to free sk_filter (v2)
From: Andrey Vagin @ 2013-11-07 4:35 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, Andrey Vagin, Alexei Starovoitov, Eric Dumazet,
David S. Miller
sk_filter isn't freed if bpf_func is equal to sk_run_filter.
This memory leak was introduced by v3.12-rc3-224-gd45ed4a4
"net: fix unsafe set_memory_rw from softirq".
Before this patch sk_filter was freed in sk_filter_release_rcu,
now it should be freed in bpf_jit_free.
Here is output of kmemleak:
unreferenced object 0xffff8800b774eab0 (size 128):
comm "systemd", pid 1, jiffies 4294669014 (age 124.062s)
hex dump (first 32 bytes):
00 00 00 00 0b 00 00 00 20 63 7f b7 00 88 ff ff ........ c......
60 d4 55 81 ff ff ff ff 30 d9 55 81 ff ff ff ff `.U.....0.U.....
backtrace:
[<ffffffff816444be>] kmemleak_alloc+0x4e/0xb0
[<ffffffff811845af>] __kmalloc+0xef/0x260
[<ffffffff81534028>] sock_kmalloc+0x38/0x60
[<ffffffff8155d4dd>] sk_attach_filter+0x5d/0x190
[<ffffffff815378a1>] sock_setsockopt+0x991/0x9e0
[<ffffffff81531bd6>] SyS_setsockopt+0xb6/0xd0
[<ffffffff8165f3e9>] system_call_fastpath+0x16/0x1b
[<ffffffffffffffff>] 0xffffffffffffffff
v2: add extra { } after else
Fixes: d45ed4a4e33a ("net: fix unsafe set_memory_rw from softirq")
Acked-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
arch/x86/net/bpf_jit_comp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 516593e..26328e8 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -788,5 +788,7 @@ void bpf_jit_free(struct sk_filter *fp)
if (fp->bpf_func != sk_run_filter) {
INIT_WORK(&fp->work, bpf_jit_free_deferred);
schedule_work(&fp->work);
+ } else {
+ kfree(fp);
}
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] ipv6: replace RTF_ROUTEINFO with RTF_ADDRCONF in rt6_get_route_info()
From: Duan Jiong @ 2013-11-07 4:19 UTC (permalink / raw)
To: hannes; +Cc: David Miller, netdev
In-Reply-To: <20131107024202.GH8144@order.stressinduktion.org>
于 2013年11月07日 10:42, Hannes Frederic Sowa 写道:
> On Thu, Nov 07, 2013 at 10:01:27AM +0800, Duan Jiong wrote:
>> 于 2013年11月07日 09:51, Hannes Frederic Sowa 写道:
>>> On Wed, Nov 06, 2013 at 03:26:41PM +0800, Duan Jiong wrote:
>>>>
>>>> As the rfc 4191 said, the Router Preference and Lifetime values in a
>>>> ::/0 Route Information Option should override the preference and lifetime
>>>> values in the Router Advertisement header. But when the kernel deals with
>>>> a ::/0 Route Information Option, the rt6_get_route_info() always return
>>>> NULL, that means that overriding will not happen, because those default
>>>> routers were added without flag RTF_ROUTEINFO in rt6_add_dflt_router().
>>>>
>>>> In order to match those default routers, we can replace RTF_ROUTEINFO
>>>> with RTF_ADDRCONF in rt6_get_route_info().
>>>>
>>>> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>>>
>>> Hmm, that looks like a bug. Nice catch!
>>>
>>> Couldn't we just replace the rt6_get_route_info in rt6_route_rcv with a call
>>> to rt6_get_dflt_router? Seems easier, already handles the ::/0 case and also
>>> does preserve the check for the RTF_ROUTEINFO flag in rt6_add_route_info.
>>
>> Yeah, your idea is better. I will modify my patch.
>
> Stop, stop, stop, sorry that suggestion is plain wrong and stupid.
>
In my opinion, i think that is well. According to your suggestion, we can
call the rt6_get_dflt_router or rt6_get_route_info in different situation.
if (rinfo->prefix_len == 0)
rt = rt6_get_dflt_router(gwaddr, dev);
else
rt = rt6_get_route_info(net, prefix, rinfo->prefix_len, gwaddr,
dev->ifindex);
How do you think of the change above?
Thanks,
Duan
> But I still would like to see the check for RTF_ROUTEINFO happening
> in rt6_add_route_info. We should also rename the function if you only query
> for RTF_ADDRCONF and not RTF_ROUTEINFO.
>
> Sorry,
>
> Hannes
>
>
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 3:05 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton
In-Reply-To: <20131107020321.GG8144@order.stressinduktion.org>
On Thu, 2013-11-07 at 03:03 +0100, Hannes Frederic Sowa wrote:
> > Oh yes, I missed a : data_len += remain;
> >
> > at line 2906 :
> >
> > offset = remain;
> > + data_len += remain;
> > continue;
>
> Hm, I still hit the WARN_ON_ONCE (same test as above) with this fixed up:
>
Thanks, I'll track this and send a v2
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Jason Wang @ 2013-11-07 2:52 UTC (permalink / raw)
To: Herbert Xu, Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst
In-Reply-To: <20131107003658.GA27976@gondor.apana.org.au>
On 11/07/2013 08:36 AM, Herbert Xu wrote:
> On Wed, Nov 06, 2013 at 07:01:10AM -0800, Eric Dumazet wrote:
>> Have you thought about arches having PAGE_SIZE=65536, and how bad it is
>> to use a full page per network frame ? It is lazy and x86 centered.
> So instead if we were sending a full 64K packet on such an arch to
> another guest, we'd now chop it up into 1.5K chunks and reassemble them.
>
>> So after our patches, we now have an optimal situation, even on these
>> arches.
> Optimal only for physical incoming packets with no jumbo frames.
>
> What's worse, I now realise that the coalesce thing isn't even
> guaranteed to work. It probably works in your benchmarks because
> you're working with freshly allocated pages.
>
> But once the system has been running for a while, I see nothing
> in the virtio_net code that tries to prevent fragmentation. Once
> fragmentation sets in, you'll be back in the terrible situation
> that we were in prior to the coalesce patch.
>
> Jason/Michael (Tsirkin), am I missing something that would prevent
> fragmentation of these buffers?
>
> Cheers,
No. Maybe we can use per-queue buffers instead.
^ permalink raw reply
* Re: [PATCH] ipv6: replace RTF_ROUTEINFO with RTF_ADDRCONF in rt6_get_route_info()
From: Hannes Frederic Sowa @ 2013-11-07 2:42 UTC (permalink / raw)
To: Duan Jiong; +Cc: David Miller, netdev
In-Reply-To: <527AF477.1060604@cn.fujitsu.com>
On Thu, Nov 07, 2013 at 10:01:27AM +0800, Duan Jiong wrote:
> 于 2013年11月07日 09:51, Hannes Frederic Sowa 写道:
> > On Wed, Nov 06, 2013 at 03:26:41PM +0800, Duan Jiong wrote:
> >>
> >> As the rfc 4191 said, the Router Preference and Lifetime values in a
> >> ::/0 Route Information Option should override the preference and lifetime
> >> values in the Router Advertisement header. But when the kernel deals with
> >> a ::/0 Route Information Option, the rt6_get_route_info() always return
> >> NULL, that means that overriding will not happen, because those default
> >> routers were added without flag RTF_ROUTEINFO in rt6_add_dflt_router().
> >>
> >> In order to match those default routers, we can replace RTF_ROUTEINFO
> >> with RTF_ADDRCONF in rt6_get_route_info().
> >>
> >> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> >
> > Hmm, that looks like a bug. Nice catch!
> >
> > Couldn't we just replace the rt6_get_route_info in rt6_route_rcv with a call
> > to rt6_get_dflt_router? Seems easier, already handles the ::/0 case and also
> > does preserve the check for the RTF_ROUTEINFO flag in rt6_add_route_info.
>
> Yeah, your idea is better. I will modify my patch.
Stop, stop, stop, sorry that suggestion is plain wrong and stupid.
But I still would like to see the check for RTF_ROUTEINFO happening
in rt6_add_route_info. We should also rename the function if you only query
for RTF_ADDRCONF and not RTF_ROUTEINFO.
Sorry,
Hannes
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Herbert Xu @ 2013-11-07 2:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <1383791855.9412.5.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 06:37:35PM -0800, Eric Dumazet wrote:
>
> If the hypervisor is doomed, there is nothing we can do.
It's not just the hypervisor you know. There is also DMA etc.
The hypervisor doesn't know that there is stuff in the page that
should not be exposed.
Also one day we may want to do zero-copy receive in which case
this would be even more important.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Alistair Popple @ 2013-11-07 2:39 UTC (permalink / raw)
To: Ben Hutchings
Cc: Benjamin Herrenschmidt, netdev, linuxppc-dev, David S. Miller
In-Reply-To: <1383756010.1520.5.camel@bwh-desktop.uk.level5networks.com>
On Wed, 6 Nov 2013 16:40:10 Ben Hutchings wrote:
> On Wed, 2013-11-06 at 12:34 +1100, Alistair Popple wrote:
> > On Tue, 5 Nov 2013 23:11:50 Ben Hutchings wrote:
> > > On Wed, 2013-11-06 at 06:54 +1100, Benjamin Herrenschmidt wrote:
> > [snip]
> >
> > > > It's an SoC bit so there's little point making it generally
> > > > selectable by the user.
> > >
> > > I think a better way to do this is:
> > >
> > > config IBM_EMAC_RGMII_WOL
> > >
> > > bool "IBM EMAC RGMII wake-on-LAN support"
> > > depends on MY_WONDERFUL_NEW_SOC || COMPILE_TEST
> > > default y if MY_WONDERFUL_NEW_SOC
> > >
> > > Then anyone making an API change that affects this driver can check that
> > > it still complies.
> >
> > The method used in this patch is the same as what is currently used by the
> > other IBM EMAC PHY interfaces (eg. config IBM_EMAC_ZMII etc). I'm happy to
> > send a patch to update all of those as well for consistency but that would
> > mean adding what each platform requires into EMACS Kconfig as well.
> >
> > Personally I think it is nicer to keep the definitions of what each
> > platform requires in one place (ie. arch/powerpc/platforms/44x/Kconfig)
> > as it is consistent with what we do for other 44x drivers, however I am
> > happy to use the above method if people think it's better.
>
> Yes, I see your point.
>
> > Alternatively we could do something like this:
> >
> > config IBM_EMAC_RGMII_WOL
> >
> > bool
> > default y if COMPILE_TEST
> > default n
> >
> > This would leave the platform dependencies as they are currently but still
> > allow compile testing.
>
> It still shouldn't default to y in that case. Instead you can make the
> symbol conditionally configurable:
>
> config IBM_EMAC_RGMII_WOL
> bool "IBM EMAC RGMII wake-on-LAN support" if COMPILE_TEST
>
> and then select this from your platform Kconfig as you intended.
That looks reasonable - I will include it in the next version of the patch
series. Thanks.
> (There is no need to put 'default n' as that's implicit for a
> configurable symbol. But it doesn't hurt either.)
>
> Ben.
Alistair
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 2:37 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <20131107021519.GA29081@gondor.apana.org.au>
On Thu, 2013-11-07 at 10:15 +0800, Herbert Xu wrote:
> On Wed, Nov 06, 2013 at 06:02:38PM -0800, Eric Dumazet wrote:
> >
> > 4K page will contain 2 frags and they will coalesce.
> >
> > Performance will still be quite good.
> >
> > We probably add a tweak, to not have any hole in this case.
>
> Also have you considered the security aspect of this? If you have
> two skbs sharing a page, and one gets transmitted to a third party
> using zero-copy, the other unrelated skb's content may become visible
> where it shouldn't.
If the hypervisor is doomed, there is nothing we can do.
virtio_net owns the pages, and relies on hypervisor doing the right
thing.
That you use part of the page, is really irrelevant.
It seems you are speaking of virtio_net sending frames, but its about
receiving frames here.
We receive frames, delivered by the trusted hypervisor.
OK, I will shut up now, since apparently I really upset you.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox