for you to fetch changes up to 67438dd33e0bb1e2f120f4f6672b5c43d30ce5fd:
sfc: Fix maximum number of TSO segments and minimum TX queue size
(2012-08-23 10:07:17 -0600)
----------------------------------------------------------------
Ben Hutchings (4):
net: Allow driver to limit number of GSO segments per skb
tcp: Apply device TSO segment limit earlier
sfc: Replace some literal constants with EFX_PAGE_SIZE/EFX_BUF_SIZE
sfc: Fix maximum number of TSO segments and minimum TX queue size
Neal Cardwell (1):
tcp: do not scale TSO segment size with reordering degree
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
08-23-2012, 07:24 PM
Herton Ronaldo Krzesinski
Lucid CVE-2012-3412
* patch "net: Allow driver to limit number of GSO segments per skb"
I think the backport is wrong. On Lucid there is no netif_skb_features,
but the check shouldn't go inside dev_can_checksum. As far as I
understand the code changes, the clearing GSO flags in dev->features
should happen right before netif_needs_gso, and on Lucid netif_needs_gso
is called from two places instead of one (also on xen-netfront), so may
be for Lucid the clearing of the feature flags could be done inside
netif_needs_gso.
I didn't paid attention before, but I noticed now that on backports of
this patch, on tcp_is_cwnd_limited, you are returning a bool on the
backport while the function was still of int type, shouldn't cause
problems but looks ugly. It happened on all backports of this patch
since Precise, so if fixing this here on Lucid, probably should be fixed
on the others as well (Natty->Precise).
Also other thing that was not part of the change in the commit upstream,
and is harmless anyway, is that on tcp_mss_split_point, you change
"struct tcp_sock *tp" declaration to const. I don't think is a problem
anyway, but isn't related to the change. It wasn't const in Oneiric and
earlier.
* patch "sfc: Fix maximum number of TSO segments and minimum TX queue size"
On this last one, I saw you define new macros,
EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE, but they are not used on the
backport, is that needed? Also EFX_TXQ_SIZE is changed, but doesn't seem
needed as well on the backport.
--
[]'s
Herton
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
08-23-2012, 08:14 PM
Tim Gardner
Lucid CVE-2012-3412
On 08/23/2012 01:24 PM, Herton Ronaldo Krzesinski wrote:
> * patch "net: Allow driver to limit number of GSO segments per skb"
>
> I think the backport is wrong. On Lucid there is no netif_skb_features,
> but the check shouldn't go inside dev_can_checksum. As far as I
> understand the code changes, the clearing GSO flags in dev->features
> should happen right before netif_needs_gso, and on Lucid netif_needs_gso
> is called from two places instead of one (also on xen-netfront), so may
> be for Lucid the clearing of the feature flags could be done inside
> netif_needs_gso.
>
You are correct. I didn't look at that one close enough.
> * patch "tcp: Apply device TSO segment limit earlier"
>
> I didn't paid attention before, but I noticed now that on backports of
> this patch, on tcp_is_cwnd_limited, you are returning a bool on the
> backport while the function was still of int type, shouldn't cause
> problems but looks ugly. It happened on all backports of this patch
> since Precise, so if fixing this here on Lucid, probably should be fixed
> on the others as well (Natty->Precise).
>
The code is functionally equivalent, and is true to the upstream patch
so I'm inclined to leave it be.
> Also other thing that was not part of the change in the commit upstream,
> and is harmless anyway, is that on tcp_mss_split_point, you change
> "struct tcp_sock *tp" declaration to const. I don't think is a problem
> anyway, but isn't related to the change. It wasn't const in Oneiric and
> earlier.
>
The upstream prototype is 'const struct tcp_sock *tp', but I think its
benign.
> * patch "sfc: Fix maximum number of TSO segments and minimum TX queue size"
>
> On this last one, I saw you define new macros,
> EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE, but they are not used on the
> backport, is that needed? Also EFX_TXQ_SIZE is changed, but doesn't seem
> needed as well on the backport.
>
EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE came in with the patch backport and
are indeed unused (and benign).
I changed both EFX_TXQ_SIZE and EFX_RXQ_SIZE to use
EFX_DEFAULT_DMAQ_SIZE so that neither is a magic number.
I've repushed with an update to "net: Allow driver to limit number of
GSO segments per skb"
rtg
--
Tim Gardner tim.gardner@canonical.com
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
08-24-2012, 12:46 AM
Herton Ronaldo Krzesinski
Lucid CVE-2012-3412
On Thu, Aug 23, 2012 at 02:14:23PM -0600, Tim Gardner wrote:
[...]
> I've repushed with an update to "net: Allow driver to limit number of
> GSO segments per skb"
This one still doesn't look ok. You clear dev->features, but the
behaviour of the upstream patch is different: they copy dev->features to
a local variable, and clear that, passing to netif_needs_gso, otherwise
after you clear first time, dev->features will have always the flags
cleared, and the checks are per skb, shouldn't be cleared globally in
dev->features. Fixing this, Ack from me for the patch series, the other
things I just wanted to point out but should be benign or style issues.
>
> rtg
> --
> Tim Gardner tim.gardner@canonical.com
>
--
[]'s
Herton
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
08-24-2012, 01:58 PM
Tim Gardner
Lucid CVE-2012-3412
This is a multi-part message in MIME format.On 08/23/2012 06:46 PM, Herton Ronaldo Krzesinski wrote:
> On Thu, Aug 23, 2012 at 02:14:23PM -0600, Tim Gardner wrote:
> [...]
>> I've repushed with an update to "net: Allow driver to limit number of
>> GSO segments per skb"
>
> This one still doesn't look ok. You clear dev->features, but the
> behaviour of the upstream patch is different: they copy dev->features to
> a local variable, and clear that, passing to netif_needs_gso, otherwise
> after you clear first time, dev->features will have always the flags
> cleared, and the checks are per skb, shouldn't be cleared globally in
> dev->features. Fixing this, Ack from me for the patch series, the other
> things I just wanted to point out but should be benign or style issues.
>
Doh, how about this? Also repushed.
rtg
--
Tim Gardner tim.gardner@canonical.com
08-24-2012, 03:05 PM
Herton Ronaldo Krzesinski
Lucid CVE-2012-3412
On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
> {
> + if (skb_is_gso(skb) &&
> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
> + return 0;
Shouldn't be return 1 here? If the condition is true, we would clear the
flags from features. If flags are cleared, when calling skb_gso_ok:
net_gso_ok would always return 0
skb_gso_ok would always return 0
netif_needs_gso returns 1 because it does !skb_gso_ok
Unless I'm missing something here. Anyway, hard to read these functions...
I think just copying/clearing the flags and passing through skb_gso_ok
would be better.
--
[]'s
Herton
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
08-24-2012, 03:11 PM
Tim Gardner
Lucid CVE-2012-3412
On 08/24/2012 09:05 AM, Herton Ronaldo Krzesinski wrote:
> On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
>> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
>> {
>> + if (skb_is_gso(skb) &&
>> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
>> + return 0;
>
> Shouldn't be return 1 here? If the condition is true, we would clear the
> flags from features. If flags are cleared, when calling skb_gso_ok:
>
> net_gso_ok would always return 0
> skb_gso_ok would always return 0
> netif_needs_gso returns 1 because it does !skb_gso_ok
>
> Unless I'm missing something here. Anyway, hard to read these functions...
> I think just copying/clearing the flags and passing through skb_gso_ok
> would be better.
>
I guess I'm confused about when the flag is set. I was assuming if GSO
was set, then the driver handled 'generic segmentation offload'. Aren't
we checking for error conditions, e.g., if there are more segments then
the driver can handle, then _don't_ do GSO ?
rtg
--
Tim Gardner tim.gardner@canonical.com
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
08-24-2012, 07:38 PM
Tim Gardner
Lucid CVE-2012-3412
On 08/24/2012 09:11 AM, Tim Gardner wrote:
> On 08/24/2012 09:05 AM, Herton Ronaldo Krzesinski wrote:
>> On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
>>> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
>>> {
>>> + if (skb_is_gso(skb) &&
>>> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
>>> + return 0;
>>
>> Shouldn't be return 1 here? If the condition is true, we would clear the
>> flags from features. If flags are cleared, when calling skb_gso_ok:
>>
>> net_gso_ok would always return 0
>> skb_gso_ok would always return 0
>> netif_needs_gso returns 1 because it does !skb_gso_ok
>>
>> Unless I'm missing something here. Anyway, hard to read these functions...
>> I think just copying/clearing the flags and passing through skb_gso_ok
>> would be better.
>>
>
> I guess I'm confused about when the flag is set. I was assuming if GSO
> was set, then the driver handled 'generic segmentation offload'. Aren't
> we checking for error conditions, e.g., if there are more segments then
> the driver can handle, then _don't_ do GSO ?
>
> rtg
>
After some online discussion with Herton I think we've agreed to bag
this mess. This is not a broad vulnerability, and the patch is proving
to be more effort then its worth.
rtg
--
Tim Gardner tim.gardner@canonical.com
--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
08-24-2012, 07:50 PM
Herton Ronaldo Krzesinski
Lucid CVE-2012-3412
On Fri, Aug 24, 2012 at 09:11:22AM -0600, Tim Gardner wrote:
> On 08/24/2012 09:05 AM, Herton Ronaldo Krzesinski wrote:
> > On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
> >> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
> >> {
> >> + if (skb_is_gso(skb) &&
> >> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
> >> + return 0;
> >
> > Shouldn't be return 1 here? If the condition is true, we would clear the
> > flags from features. If flags are cleared, when calling skb_gso_ok:
> >
> > net_gso_ok would always return 0
> > skb_gso_ok would always return 0
> > netif_needs_gso returns 1 because it does !skb_gso_ok
> >
> > Unless I'm missing something here. Anyway, hard to read these functions...
> > I think just copying/clearing the flags and passing through skb_gso_ok
> > would be better.
> >
>
> I guess I'm confused about when the flag is set. I was assuming if GSO
> was set, then the driver handled 'generic segmentation offload'. Aren't
> we checking for error conditions, e.g., if there are more segments then
> the driver can handle, then _don't_ do GSO ?
Ok, forget last thing I said on IRC about sfc, it is the only one
limiting the gso_max_segs, the others which supports GSO continues to do
so and use the default maximum of GSO_MAX_SEGS as defined on the patch.
Looks like only sfc indeed needs this and is affected, as it's my
understanding.
About the patch, if still pursuing this, I expect the following diff
should work for Lucid. I think I handled what's needed. I noticed
skb_gso_ok was used at dev_gso_segment()->skb_gso_segment(), so we
can't patch netif_needs_gso directly, and if we skb_gso_ok is used with
protocol code so we couldn't clear flags there (not sure if there would
be a side effect doing so).
+int netif_skb_features(struct net_device *dev, struct sk_buff *skb)
+{
+ int features = dev->features;
+
+ if (skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
+ features &= ~NETIF_F_GSO_MASK;
+
+ return features;
+}
+EXPORT_SYMBOL(netif_skb_features);
+
/*
* Invalidate hardware checksum when packet is to be mangled, and
* complete checksum manually on outgoing path.
@@ -1682,13 +1693,12 @@ static void dev_gso_skb_destructor(struct sk_buff *skb)
* This function segments the given skb and stores the list of segments
* in skb->next.
*/
-static int dev_gso_segment(struct sk_buff *skb)
+static int dev_gso_segment(struct sk_buff *skb, int features)
{
struct net_device *dev = skb->dev;
struct sk_buff *segs;
- int features = dev->features & ~(illegal_highdma(dev, skb) ?
- NETIF_F_SG : 0);
/* Verifying header integrity only. */
@@ -1712,11 +1722,15 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
int rc;
if (likely(!skb->next)) {
+ int features;
+
if (!list_empty(&ptype_all))
dev_queue_xmit_nit(skb, dev);
- if (netif_needs_gso(dev, skb)) {
- if (unlikely(dev_gso_segment(skb)))
+ features = netif_skb_features(dev, skb);
+
+ if (netif_needs_gso(skb, features)) {
+ if (unlikely(dev_gso_segment(skb, features)))
goto out_kfree_skb;
if (skb->next)
goto gso;
@@ -1887,7 +1901,7 @@ int dev_queue_xmit(struct sk_buff *skb)
int rc = -ENOMEM;
/* GSO will handle the following emulations directly. */
- if (netif_needs_gso(dev, skb))
+ if (netif_needs_gso(skb, netif_skb_features(dev, skb)))
goto gso;
Lucid CVE-2012-3412
