Patchwork Lucid CVE-2012-3412

login
register
mail settings
Submitter Tim Gardner
Date Aug. 23, 2012, 4:27 p.m.
Message ID <503659DD.6070708@canonical.com>
Download mbox
Permalink /patch/179680/
State New
Headers show

Pull-request

git://kernel.ubuntu.com/rtg/ubuntu-lucid.git CVE-2012-3412

Comments

Tim Gardner - Aug. 23, 2012, 4:27 p.m.
The following changes since commit cf52d980c29eade382d95198b7362787038b9ef5:

  eCryptfs: Initialize empty lower files when opening them (2012-08-23
12:05:04 -0400)

are available in the git repository at:

  git://kernel.ubuntu.com/rtg/ubuntu-lucid.git CVE-2012-3412

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

 drivers/net/sfc/efx.c     |    8 ++++++++
 drivers/net/sfc/efx.h     |   19 +++++++++++++++++--
 drivers/net/sfc/nic.c     |    5 +----
 drivers/net/sfc/nic.h     |    5 +++++
 drivers/net/sfc/tx.c      |   21 ++++++++++++++++++++-
 include/linux/netdevice.h |    2 ++
 include/net/sock.h        |    2 ++
 include/net/tcp.h         |    8 ++++++++
 net/core/dev.c            |    4 ++++
 net/core/sock.c           |    1 +
 net/ipv4/tcp.c            |    4 +++-
 net/ipv4/tcp_cong.c       |    7 ++++---
 net/ipv4/tcp_output.c     |   27 +++++++++++++++------------
 13 files changed, 90 insertions(+), 23 deletions(-)
Herton Ronaldo Krzesinski - Aug. 23, 2012, 7:24 p.m.
* 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.

* 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).

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.
Tim Gardner - Aug. 23, 2012, 8:14 p.m.
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
Herton Ronaldo Krzesinski - Aug. 24, 2012, 12:46 a.m.
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
>