Netdev List
 help / color / mirror / Atom feed
* [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
From: Guenter Roeck @ 2013-02-27 19:46 UTC (permalink / raw)
  To: netdev; +Cc: David S. Miller, Guenter Roeck

Building af_packet may fail with

In function ‘copy_from_user’,
    inlined from ‘packet_getsockopt’ at
    net/packet/af_packet.c:3215:21:
arch/x86/include/asm/uaccess_32.h:211:26: error: call to
    ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
    buffer size is not provably correct

if built with W=1 due to a missing parameter size validation.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 net/packet/af_packet.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c7bfeff..1976b23 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
 		val = po->tp_version;
 		break;
 	case PACKET_HDRLEN:
+		if (len < sizeof(int))
+			return -EINVAL;
 		if (len > sizeof(int))
 			len = sizeof(int);
 		if (copy_from_user(&val, optval, len))
-- 
1.7.9.7

^ permalink raw reply related

* Re: [RFC PATCH 0/5] net: low latency Ethernet device polling
From: Rick Jones @ 2013-02-27 19:58 UTC (permalink / raw)
  To: Eliezer Tamir
  Cc: linux-kernel, netdev, Dave Miller, Jesse Brandeburg, e1000-devel,
	Willem de Bruijn, Andi Kleen, HPA, Eliezer Tamir
In-Reply-To: <20130227175549.10611.82188.stgit@gitlad.jf.intel.com>

On 02/27/2013 09:55 AM, Eliezer Tamir wrote:
> This patchset adds the ability for the socket layer code to poll directly
> on an Ethernet device's RX queue. This eliminates the cost of the interrupt
> and context switch and with proper tuning allows us to get very close
> to the HW latency.
>
> This is a follow up to Jesse Brandeburg's Kernel Plumbers talk from last year
> http://www.linuxplumbersconf.org/2012/wp-content/uploads/2012/09/2012-lpc-Low-Latency-Sockets-slides-brandeburg.pdf
>
> Patch 1 adds ndo_ll_poll and the IP code to use it.
> Patch 2 is an example of how TCP can use ndo_ll_poll.
> Patch 3 shows how this method would be implemented for the ixgbe driver.
> Patch 4 adds statistics to the ixgbe driver for ndo_ll_poll events.
> (Optional) Patch 5 is a handy kprobes module to measure detailed latency
> numbers.
>
> this patchset is also available in the following git branch
> git://github.com/jbrandeb/lls.git rfc
>
> Performance numbers:
> Kernel   Config     C3/6  rx-usecs  TCP  UDP
> 3.8rc6   typical    off   adaptive  37k  40k
> 3.8rc6   typical    off   0*        50k  56k
> 3.8rc6   optimized  off   0*        61k  67k
> 3.8rc6   optimized  on    adaptive  26k  29k
> patched  typical    off   adaptive  70k  78k
> patched  optimized  off   adaptive  79k  88k
> patched  optimized  off   100       84k  92k
> patched  optimized  on    adaptive  83k  91k
> *rx-usecs=0 is usually not useful in a production environment.

I would think that latency-sensitive folks would be using rx-usecs=0 in 
production - at least if the NIC in use didn't have low enough latency 
with its default interrupt coalescing/avoidance heuristics.

If I take the first "pure" A/B comparison it seems that the change as 
benchmarked takes latency for TCP from ~27 usec (37k) to ~14 usec (70k). 
  At what request/response size does the benefit taper-off?  13 usec 
seems to be about 16250 bytes at 10 GbE.

When I last looked at netperf TCP_RR performance where something similar 
could happen I think it was IPoIB where it was possible to set things up 
such that polling happened rather than wakeups (perhaps it was with a 
shim library that converted netperf's socket calls to "native" IB).  My 
recollection is that it "did a number" on the netperf service demands 
thanks to the spinning.  It would be a good thing to include those 
figures in any subsequent rounds of benchmarking.

Am I correct in assuming this is a mechanism which would not be used in 
a high aggregate PPS situation?

happy benchmarking,

rick jones

^ permalink raw reply

* Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message
From: Neil Horman @ 2013-02-27 20:09 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: netdev, linux-sctp, Vlad Yasevich, Sridhar Samudrala,
	David S. Miller
In-Reply-To: <1361994231-5355-1-git-send-email-linux@roeck-us.net>

On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> Building sctp may fail with:
> 
> In function ‘copy_from_user’,
>     inlined from ‘sctp_getsockopt_assoc_stats’ at
>     net/sctp/socket.c:5656:20:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>     buffer size is not provably correct
> 
> if built with W=1 due to a missing parameter size validation.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  net/sctp/socket.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index cedd9bf..0a5f2bf 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>  	/* User must provide at least the assoc id */
>  	if (len < sizeof(sctp_assoc_t))
>  		return -EINVAL;
> +	if (len > sizeof(struct sctp_assoc_stats))
> +		len = sizeof(struct sctp_assoc_stats);
>  
>  	if (copy_from_user(&sas, optval, len))
>  		return -EFAULT;
> -- 
> 1.7.9.7
> 
> 

Theres more than that going on here.  This will fix the warning, but the
function is written such that, if you pass in a size that is greater than the
size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
not sure that a partial stat struct is really that useful to people.  What if
you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
your minimum length check, then just did a copy_from_user of that length.  It
would save you having to compute two lengths separately, since you could then
just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
that function.

Thanks!
Neil

^ permalink raw reply

* [PATCH] rtlwifi: rtl8192cu: Fix schedule while atomic bug splat
From: Larry Finger @ 2013-02-27 20:10 UTC (permalink / raw)
  To: John W Linville, jussi.kivilinna; +Cc: netdev, linux-wireless

When run at debug 3 or higher, rtl8192cu reports a BUG as follows:

BUG: scheduling while atomic: kworker/u:0/5281/0x00000002
INFO: lockdep is turned off.
Modules linked in: rtl8192cu rtl8192c_common rtlwifi fuse af_packet bnep bluetooth b43 mac80211 cfg80211 ipv6 snd_hda_codec_conexant kvm_amd k
vm snd_hda_intel snd_hda_codec bcma rng_core snd_pcm ssb mmc_core snd_seq snd_timer snd_seq_device snd i2c_nforce2 sr_mod pcmcia forcedeth i2c_core soundcore
 cdrom sg serio_raw k8temp hwmon joydev ac battery pcmcia_core snd_page_alloc video button wmi autofs4 ext4 mbcache jbd2 crc16 thermal processor scsi_dh_alua
 scsi_dh_hp_sw scsi_dh_rdac scsi_dh_emc scsi_dh ata_generic pata_acpi pata_amd [last unloaded: rtlwifi]
Pid: 5281, comm: kworker/u:0 Tainted: G        W    3.8.0-wl+ #119
Call Trace:
 [<ffffffff814531e7>] __schedule_bug+0x62/0x70
 [<ffffffff81459af0>] __schedule+0x730/0xa30
 [<ffffffff81326e49>] ? usb_hcd_link_urb_to_ep+0x19/0xa0
 [<ffffffff8145a0d4>] schedule+0x24/0x70
 [<ffffffff814575ec>] schedule_timeout+0x18c/0x2f0
 [<ffffffff81459ec0>] ? wait_for_common+0x40/0x180
 [<ffffffff8133f461>] ? ehci_urb_enqueue+0xf1/0xee0
 [<ffffffff810a579d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81459f65>] wait_for_common+0xe5/0x180
 [<ffffffff8107d1c0>] ? try_to_wake_up+0x2d0/0x2d0
 [<ffffffff8145a08e>] wait_for_completion_timeout+0xe/0x10
 [<ffffffff8132ab1c>] usb_start_wait_urb+0x8c/0x100
 [<ffffffff8132adf9>] usb_control_msg+0xd9/0x130
 [<ffffffffa057dd8d>] _usb_read_sync+0xcd/0x140 [rtlwifi]
 [<ffffffffa057de0e>] _usb_read32_sync+0xe/0x10 [rtlwifi]
 [<ffffffffa04b0555>] rtl92cu_update_hal_rate_table+0x1a5/0x1f0 [rtl8192cu]

The cause is a synchronous read from routine rtl92cu_update_hal_rate_table().
The resulting output is not critical, thus the debug statement is
deleted.

Reported-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Stable <stable@vger.kernel.org>
---

Index: wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
+++ wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
@@ -2058,8 +2058,6 @@ void rtl92cu_update_hal_rate_table(struc
 			       (shortgi_rate << 4) | (shortgi_rate);
 	}
 	rtl_write_dword(rtlpriv, REG_ARFR0 + ratr_index * 4, ratr_value);
-	RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG, "%x\n",
-		 rtl_read_dword(rtlpriv, REG_ARFR0));
 }
 
 void rtl92cu_update_hal_rate_mask(struct ieee80211_hw *hw, u8 rssi_level)

^ permalink raw reply

* Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic
From: Vlad Yasevich @ 2013-02-27 20:13 UTC (permalink / raw)
  To: Lee A. Roberts; +Cc: netdev
In-Reply-To: <1361991273-30578-1-git-send-email-lee.roberts@hp.com>

On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.

Hi Lee

What changed in this series?  I looked through and they don't look 
different from the prior version?

-vlad

>
> Lee A. Roberts (4):
>    sctp: fix association hangs due to off-by-one errors in
>      sctp_tsnmap_grow()
>    sctp: fix association hangs due to reneging packets below the
>      cumulative TSN ACK point
>    sctp: fix association hangs due to errors when reneging events from
>      the ordering queue
>    sctp: fix association hangs due to partial delivery errors
>
>   net/sctp/tsnmap.c   |   13 ++++----
>   net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
>   2 files changed, 78 insertions(+), 22 deletions(-)
>

^ permalink raw reply

* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
From: David Miller @ 2013-02-27 20:19 UTC (permalink / raw)
  To: linux; +Cc: netdev
In-Reply-To: <1361994418-1403-1-git-send-email-linux@roeck-us.net>


The first thing this function does is test whether len < 0, therefore
your change is unnecessary.

If the user gives us something between 0 and sizeof(int), that's
their problem, and they'll get a partial int copied back into
userspace as a result instead of the complete integer.

Please don't blindly silence warnings like this, thanks.

^ permalink raw reply

* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
From: Daniel Borkmann @ 2013-02-27 20:22 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: netdev, David S. Miller
In-Reply-To: <1361994418-1403-1-git-send-email-linux@roeck-us.net>

On 02/27/2013 08:46 PM, Guenter Roeck wrote:
> Building af_packet may fail with
>
> In function ‘copy_from_user’,
>      inlined from ‘packet_getsockopt’ at
>      net/packet/af_packet.c:3215:21:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>      ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>      buffer size is not provably correct
>
> if built with W=1 due to a missing parameter size validation.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   net/packet/af_packet.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c7bfeff..1976b23 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
>   		val = po->tp_version;
>   		break;
>   	case PACKET_HDRLEN:
> +		if (len < sizeof(int))
> +			return -EINVAL;

I think this could break some user space applications here, those who e.g. only pass
an uint16_t to packet_getsockopt with PACKET_HDRLEN.

>   		if (len > sizeof(int))
>   			len = sizeof(int);
>   		if (copy_from_user(&val, optval, len))

^ permalink raw reply

* Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message
From: David Miller @ 2013-02-27 20:22 UTC (permalink / raw)
  To: nhorman; +Cc: linux, netdev, linux-sctp, vyasevich, sri
In-Reply-To: <20130227200931.GB7993@hmsreliant.think-freely.org>

From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 27 Feb 2013 15:09:31 -0500

> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
>> Building sctp may fail with:
>> 
>> In function ‘copy_from_user’,
>>     inlined from ‘sctp_getsockopt_assoc_stats’ at
>>     net/sctp/socket.c:5656:20:
>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>>     buffer size is not provably correct
>> 
>> if built with W=1 due to a missing parameter size validation.
>> 
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  net/sctp/socket.c |    2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index cedd9bf..0a5f2bf 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>>  	/* User must provide at least the assoc id */
>>  	if (len < sizeof(sctp_assoc_t))
>>  		return -EINVAL;
>> +	if (len > sizeof(struct sctp_assoc_stats))
>> +		len = sizeof(struct sctp_assoc_stats);
>>  
>>  	if (copy_from_user(&sas, optval, len))
>>  		return -EFAULT;
>> -- 
>> 1.7.9.7
>> 
>> 
> 
> Theres more than that going on here.  This will fix the warning, but the
> function is written such that, if you pass in a size that is greater than the
> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> not sure that a partial stat struct is really that useful to people.  What if
> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> your minimum length check, then just did a copy_from_user of that length.  It
> would save you having to compute two lengths separately, since you could then
> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> that function.

Genreally, getsockopt() implementations happily give partial return values
when the user gives a too small length.

^ permalink raw reply

* Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message
From: Guenter Roeck @ 2013-02-27 20:22 UTC (permalink / raw)
  To: Neil Horman
  Cc: netdev, linux-sctp, Vlad Yasevich, Sridhar Samudrala,
	David S. Miller
In-Reply-To: <20130227200931.GB7993@hmsreliant.think-freely.org>

On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> > Building sctp may fail with:
> > 
> > In function ‘copy_from_user’,
> >     inlined from ‘sctp_getsockopt_assoc_stats’ at
> >     net/sctp/socket.c:5656:20:
> > arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> >     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
> >     buffer size is not provably correct
> > 
> > if built with W=1 due to a missing parameter size validation.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  net/sctp/socket.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index cedd9bf..0a5f2bf 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
> >  	/* User must provide at least the assoc id */
> >  	if (len < sizeof(sctp_assoc_t))
> >  		return -EINVAL;
> > +	if (len > sizeof(struct sctp_assoc_stats))
> > +		len = sizeof(struct sctp_assoc_stats);
> >  
> >  	if (copy_from_user(&sas, optval, len))
> >  		return -EFAULT;
> > -- 
> > 1.7.9.7
> > 
> > 
> 
> Theres more than that going on here.  This will fix the warning, but the
> function is written such that, if you pass in a size that is greater than the
> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> not sure that a partial stat struct is really that useful to people.  What if
> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> your minimum length check, then just did a copy_from_user of that length.  It
> would save you having to compute two lengths separately, since you could then
> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> that function.
> 
Yes, but that would require input from someone who knows the code. All I am trying
to accomplish is to ensure that copy_from_user does not overwrite the stack.

Thanks,
Guenter

^ permalink raw reply

* Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic
From: David Miller @ 2013-02-27 20:24 UTC (permalink / raw)
  To: vyasevic; +Cc: lee.roberts, netdev
In-Reply-To: <512E6905.2020808@redhat.com>

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Wed, 27 Feb 2013 15:13:57 -0500

> On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
>> From: "Lee A. Roberts" <lee.roberts@hp.com>
>>
>> This series of patches resolves several SCTP association hangs
>> observed during
>> SCTP stress testing.  Observable symptoms include communications hangs
>> with
>> data being held in the association reassembly and/or lobby (ordering)
>> queues.
>> Close examination of reassembly/ordering queues may show either
>> duplicated
>> or missing packets.
> 
> Hi Lee
> 
> What changed in this series?  I looked through and they don't look
> different from the prior version?

He fixed the build failure, and he described this at:

http://marc.info/?l=linux-netdev&m=136199143224349&w=2

But yeah he absolutely and positively should have mentioned it here
too.

^ permalink raw reply

* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
From: David Miller @ 2013-02-27 20:26 UTC (permalink / raw)
  To: dborkman; +Cc: linux, netdev
In-Reply-To: <512E6AF9.3030901@redhat.com>

From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 27 Feb 2013 21:22:17 +0100

> On 02/27/2013 08:46 PM, Guenter Roeck wrote:
>> Building af_packet may fail with
>>
>> In function ‘copy_from_user’,
>>      inlined from ‘packet_getsockopt’ at
>>      net/packet/af_packet.c:3215:21:
>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>      ‘copy_from_user_overflow’ declared with attribute error:
>>      copy_from_user()
>>      buffer size is not provably correct
>>
>> if built with W=1 due to a missing parameter size validation.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   net/packet/af_packet.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index c7bfeff..1976b23 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket
>> *sock, int level, int optname,
>>   		val = po->tp_version;
>>   		break;
>>   	case PACKET_HDRLEN:
>> +		if (len < sizeof(int))
>> +			return -EINVAL;
> 
> I think this could break some user space applications here, those who
> e.g. only pass
> an uint16_t to packet_getsockopt with PACKET_HDRLEN.

Well, their shit is broken on big endian then.

^ permalink raw reply

* RE: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic
From: Roberts, Lee A. @ 2013-02-27 20:26 UTC (permalink / raw)
  To: vyasevic@redhat.com; +Cc: netdev@vger.kernel.org
In-Reply-To: <512E6905.2020808@redhat.com>

> -----Original Message-----
> From: Vlad Yasevich [mailto:vyasevic@redhat.com]
> Sent: Wednesday, February 27, 2013 1:14 PM
> To: Roberts, Lee A.
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic
> 
> On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
> > From: "Lee A. Roberts" <lee.roberts@hp.com>
> >
> > This series of patches resolves several SCTP association hangs observed during
> > SCTP stress testing.  Observable symptoms include communications hangs with
> > data being held in the association reassembly and/or lobby (ordering) queues.
> > Close examination of reassembly/ordering queues may show either duplicated
> > or missing packets.
> 
> Hi Lee
> 
> What changed in this series?  I looked through and they don't look
> different from the prior version?
> 
> -vlad
> 

Vlad,

In patch #4, routine sctp_ulpq_renege(), the inserted call to sctp_ulpq_partial_delivery()
still used the 3.2.x calling sequence with three arguments.

The first version of patch #4 stated:

+		if (retval <= 0)
+			sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
+		else if (retval == 1)
+			sctp_ulpq_reasm_drain(ulpq);

The second version of patch #4 has corrected the call to sctp_ulpq_partial_delivery():

+		if (retval <= 0)
+			sctp_ulpq_partial_delivery(ulpq, gfp);
+		else if (retval == 1)
+			sctp_ulpq_reasm_drain(ulpq);

I have adjusted the patch comments for yet another version of the patch series.
I'm holding them at the moment to collect any further change requests.

                                        - Lee



> >
> > Lee A. Roberts (4):
> >    sctp: fix association hangs due to off-by-one errors in
> >      sctp_tsnmap_grow()
> >    sctp: fix association hangs due to reneging packets below the
> >      cumulative TSN ACK point
> >    sctp: fix association hangs due to errors when reneging events from
> >      the ordering queue
> >    sctp: fix association hangs due to partial delivery errors
> >
> >   net/sctp/tsnmap.c   |   13 ++++----
> >   net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
> >   2 files changed, 78 insertions(+), 22 deletions(-)
> >

^ permalink raw reply

* Re: [PATCH] net: af_packet: Validate parameter size for PACKET_HDRLEN control message
From: Guenter Roeck @ 2013-02-27 20:33 UTC (permalink / raw)
  To: David Miller; +Cc: dborkman, netdev
In-Reply-To: <20130227.152630.35846741651175869.davem@davemloft.net>

On Wed, Feb 27, 2013 at 03:26:30PM -0500, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Wed, 27 Feb 2013 21:22:17 +0100
> 
> > On 02/27/2013 08:46 PM, Guenter Roeck wrote:
> >> Building af_packet may fail with
> >>
> >> In function ‘copy_from_user’,
> >>      inlined from ‘packet_getsockopt’ at
> >>      net/packet/af_packet.c:3215:21:
> >> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> >>      ‘copy_from_user_overflow’ declared with attribute error:
> >>      copy_from_user()
> >>      buffer size is not provably correct
> >>
> >> if built with W=1 due to a missing parameter size validation.
> >>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >>   net/packet/af_packet.c |    2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> >> index c7bfeff..1976b23 100644
> >> --- a/net/packet/af_packet.c
> >> +++ b/net/packet/af_packet.c
> >> @@ -3210,6 +3210,8 @@ static int packet_getsockopt(struct socket
> >> *sock, int level, int optname,
> >>   		val = po->tp_version;
> >>   		break;
> >>   	case PACKET_HDRLEN:
> >> +		if (len < sizeof(int))
> >> +			return -EINVAL;
> > 
> > I think this could break some user space applications here, those who
> > e.g. only pass
> > an uint16_t to packet_getsockopt with PACKET_HDRLEN.
> 
> Well, their shit is broken on big endian then.

There must be something else going on anyway ... yes, my patch fixes the
warning/error, but copy_from_user should only bail out if the copy size
can be larger than the provided buffer (unless I misunderstand the code
in copy_from_user). And the second check should take care of that.

Anyway, point taken. I'll waste my time elsewhere :).

Thanks,
Guenter

^ permalink raw reply

* Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message
From: David Miller @ 2013-02-27 20:33 UTC (permalink / raw)
  To: linux; +Cc: netdev, linux-sctp, vyasevich, sri, nhorman
In-Reply-To: <1361994231-5355-1-git-send-email-linux@roeck-us.net>

From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 27 Feb 2013 11:43:51 -0800

> Building sctp may fail with:
> 
> In function ‘copy_from_user’,
>     inlined from ‘sctp_getsockopt_assoc_stats’ at
>     net/sctp/socket.c:5656:20:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>     buffer size is not provably correct
> 
> if built with W=1 due to a missing parameter size validation.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

This change is correct, but please fix this by simply moving the:

	/* Allow the struct to grow and fill in as much as possible */
	len = min_t(size_t, len, sizeof(sas));

line higher up in the function.

And I also prefer this because:

	something testing sizeof(foo);
	if (copy_from_user(..., ..., sizeof(foo)))

must easier to audit and validate, especially in patch form.

Otherwise I have to bring the code into an editor and read the whole
function just to make sure you got the type correct.

Thanks.

^ permalink raw reply

* Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message
From: Vlad Yasevich @ 2013-02-27 20:37 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Neil Horman, netdev, linux-sctp, Sridhar Samudrala,
	David S. Miller
In-Reply-To: <20130227202255.GA31830@roeck-us.net>

On 02/27/2013 03:22 PM, Guenter Roeck wrote:
> On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
>> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
>>> Building sctp may fail with:
>>>
>>> In function ‘copy_from_user’,
>>>      inlined from ‘sctp_getsockopt_assoc_stats’ at
>>>      net/sctp/socket.c:5656:20:
>>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>>      ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>>>      buffer size is not provably correct
>>>
>>> if built with W=1 due to a missing parameter size validation.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   net/sctp/socket.c |    2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index cedd9bf..0a5f2bf 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>>>   	/* User must provide at least the assoc id */
>>>   	if (len < sizeof(sctp_assoc_t))
>>>   		return -EINVAL;
>>> +	if (len > sizeof(struct sctp_assoc_stats))
>>> +		len = sizeof(struct sctp_assoc_stats);
>>>
>>>   	if (copy_from_user(&sas, optval, len))
>>>   		return -EFAULT;
>>> --
>>> 1.7.9.7
>>>
>>>
>>
>> Theres more than that going on here.  This will fix the warning, but the
>> function is written such that, if you pass in a size that is greater than the
>> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
>> not sure that a partial stat struct is really that useful to people.  What if
>> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
>> your minimum length check, then just did a copy_from_user of that length.  It
>> would save you having to compute two lengths separately, since you could then
>> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
>> that function.
>>
> Yes, but that would require input from someone who knows the code. All I am trying
> to accomplish is to ensure that copy_from_user does not overwrite the stack.
>

The function already has the following in it:

	/* Allow the struct to grow and fill in as much as possible */
         len = min_t(size_t, len, sizeof(sas));


It would be better to move that up to before the copy_from_user.
The alternative would be to do:
	if (copy_from_user(&sas, optval, sizeof(sctp_assoc_t))

since all we are after here is an association id and nothing else.

-vlad

> Thanks,
> Guenter
>

^ permalink raw reply

* Re: [RFC PATCH 0/5] net: low latency Ethernet device polling
From: Eliezer Tamir @ 2013-02-27 20:40 UTC (permalink / raw)
  To: Rick Jones
  Cc: Eliezer Tamir, linux-kernel, netdev, Dave Miller,
	Jesse Brandeburg, e1000-devel, Willem de Bruijn, Andi Kleen, HPA,
	Eliezer Tamir
In-Reply-To: <512E654A.2010209@hp.com>

On 27/02/2013 21:58, Rick Jones wrote:
> On 02/27/2013 09:55 AM, Eliezer Tamir wrote:
>>
>> Performance numbers:
>> Kernel   Config     C3/6  rx-usecs  TCP  UDP
>> 3.8rc6   typical    off   adaptive  37k  40k
>> 3.8rc6   typical    off   0*        50k  56k
>> 3.8rc6   optimized  off   0*        61k  67k
>> 3.8rc6   optimized  on    adaptive  26k  29k
>> patched  typical    off   adaptive  70k  78k
>> patched  optimized  off   adaptive  79k  88k
>> patched  optimized  off   100       84k  92k
>> patched  optimized  on    adaptive  83k  91k
>> *rx-usecs=0 is usually not useful in a production environment.
>
> I would think that latency-sensitive folks would be using rx-usecs=0 in
> production - at least if the NIC in use didn't have low enough latency
> with its default interrupt coalescing/avoidance heuristics.

It will only work well if you have no bulk traffic on the same port as 
the low latency traffic at all.

> If I take the first "pure" A/B comparison it seems that the change as
> benchmarked takes latency for TCP from ~27 usec (37k) to ~14 usec (70k).
>   At what request/response size does the benefit taper-off?  13 usec
> seems to be about 16250 bytes at 10 GbE.

It's pretty easy to get a result of 80K+ with a little tweaking, an 
rx-usecs value of 100 with C3/6 enabled will get you that.

> When I last looked at netperf TCP_RR performance where something similar
> could happen I think it was IPoIB where it was possible to set things up
> such that polling happened rather than wakeups (perhaps it was with a
> shim library that converted netperf's socket calls to "native" IB).  My
> recollection is that it "did a number" on the netperf service demands
> thanks to the spinning.  It would be a good thing to include those
> figures in any subsequent rounds of benchmarking.

I will get service demand numbers, but we are busy polling so I can tell 
you right now that one core will be at 100%.

> Am I correct in assuming this is a mechanism which would not be used in
> a high aggregate PPS situation?

The current design has in mind situations where you want to react very 
fast to a trigger but that reaction could involve more than short 
messages. so we are willing to burn CPU cycles when there is nothing 
better to do, but we also want to work well when there is bulk traffic.
Ideally I would want the system to be smart about this and to know when 
not to allow busy polling.

> happy benchmarking,
we love netperf.

^ permalink raw reply

* [patch] sctp: check len in sctp_getsockopt_assoc_stats()
From: Dan Carpenter @ 2013-02-27 20:41 UTC (permalink / raw)
  To: Vlad Yasevich, Michele Baldessari
  Cc: Sridhar Samudrala, Neil Horman, David S. Miller, linux-sctp,
	netdev, kernel-janitors

This check is missing an upper bound.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This should go in 3.8 -stable as well.

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index c99458d..fbd8386 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
 	/* User must provide at least the assoc id */
 	if (len < sizeof(sctp_assoc_t))
 		return -EINVAL;
+	if (len > sizeof(struct sctp_assoc_stats))
+		len = sizeof(struct sctp_assoc_stats);
 
 	if (copy_from_user(&sas, optval, len))
 		return -EFAULT;

^ permalink raw reply related

* Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message
From: Neil Horman @ 2013-02-27 20:43 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: netdev, linux-sctp, Vlad Yasevich, Sridhar Samudrala,
	David S. Miller
In-Reply-To: <20130227202255.GA31830@roeck-us.net>

On Wed, Feb 27, 2013 at 12:22:55PM -0800, Guenter Roeck wrote:
> On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
> > On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> > > Building sctp may fail with:
> > > 
> > > In function ‘copy_from_user’,
> > >     inlined from ‘sctp_getsockopt_assoc_stats’ at
> > >     net/sctp/socket.c:5656:20:
> > > arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> > >     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
> > >     buffer size is not provably correct
> > > 
> > > if built with W=1 due to a missing parameter size validation.
> > > 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  net/sctp/socket.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index cedd9bf..0a5f2bf 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
> > >  	/* User must provide at least the assoc id */
> > >  	if (len < sizeof(sctp_assoc_t))
> > >  		return -EINVAL;
> > > +	if (len > sizeof(struct sctp_assoc_stats))
> > > +		len = sizeof(struct sctp_assoc_stats);
> > >  
> > >  	if (copy_from_user(&sas, optval, len))
> > >  		return -EFAULT;
> > > -- 
> > > 1.7.9.7
> > > 
> > > 
> > 
> > Theres more than that going on here.  This will fix the warning, but the
> > function is written such that, if you pass in a size that is greater than the
> > size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> > not sure that a partial stat struct is really that useful to people.  What if
> > you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> > your minimum length check, then just did a copy_from_user of that length.  It
> > would save you having to compute two lengths separately, since you could then
> > just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> > that function.
> > 
> Yes, but that would require input from someone who knows the code. All I am trying
> to accomplish is to ensure that copy_from_user does not overwrite the stack.
> 
Not really, its pretty straightforward.  Although, its kind of moot, after Daves
note I looked again, and if the size of the stats structure is less than the
association structure, everything works fine, but if the association is smaller
than the association, we'll get an EFAULT on the copy to user.  So it all works
out

Acked-by: Neil Horman <nhorman@tuxdriver.com>


Dave
> Thanks,
> Guenter
> 

^ permalink raw reply

* Re: [patch] sctp: check len in sctp_getsockopt_assoc_stats()
From: David Miller @ 2013-02-27 20:45 UTC (permalink / raw)
  To: dan.carpenter
  Cc: vyasevich, michele, sri, nhorman, linux-sctp, netdev,
	kernel-janitors
In-Reply-To: <20130227204144.GA11300@longonot.mountain>

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 27 Feb 2013 23:41:44 +0300

> This check is missing an upper bound.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This should go in 3.8 -stable as well.

There is a discussion and patch on netdev already.

^ permalink raw reply

* Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic
From: Vlad Yasevich @ 2013-02-27 20:49 UTC (permalink / raw)
  To: David Miller; +Cc: lee.roberts, netdev
In-Reply-To: <20130227.152407.610703805329225791.davem@davemloft.net>

On 02/27/2013 03:24 PM, David Miller wrote:
> From: Vlad Yasevich <vyasevic@redhat.com>
> Date: Wed, 27 Feb 2013 15:13:57 -0500
>
>> On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
>>> From: "Lee A. Roberts" <lee.roberts@hp.com>
>>>
>>> This series of patches resolves several SCTP association hangs
>>> observed during
>>> SCTP stress testing.  Observable symptoms include communications hangs
>>> with
>>> data being held in the association reassembly and/or lobby (ordering)
>>> queues.
>>> Close examination of reassembly/ordering queues may show either
>>> duplicated
>>> or missing packets.
>>
>> Hi Lee
>>
>> What changed in this series?  I looked through and they don't look
>> different from the prior version?
>
> He fixed the build failure, and he described this at:
>
> http://marc.info/?l=linux-netdev&m=136199143224349&w=2
>
> But yeah he absolutely and positively should have mentioned it here
> too.
>

Thanks.  Didn't realize it was just a build error.  Was looking for 
functionality change.

-vlad

^ permalink raw reply

* Re: iproute-3.8.0 build error on Linux ppc64
From: Denis Kirjanov @ 2013-02-27 20:53 UTC (permalink / raw)
  To: nello martuscielli; +Cc: Stephen Hemminger, netdev
In-Reply-To: <CABMX=WyV7t-RcdM103z9sqvvicxvnTtX-OW_OyL9MA5j_4ho1w@mail.gmail.com>

It's been solved by using  __SANE_USERSPACE_TYPES__ which selects the
proper type for __u64 on PowerPC:
https://patchwork.kernel.org/patch/2029141/

On 2/27/13, nello martuscielli <ppc.addon@gmail.com> wrote:
> On Wed, Feb 27, 2013 at 4:28 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>
> __omissis
>>> >
>>> > gcc -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
>>> > -Wmissing-declarations -Wold-style-definition -O2 -I../include
>>> > -DRESOLVE_HOSTNAMES -DLIBDIR=\"/usr/lib\" -DCONFDIR=\"/etc/iproute2\"
>>> > -D_GNU_SOURCE   -c -o ipntable.o ipntable.c
>>> > cc1: warnings being treated as errors
>>> > ipntable.c: In function 'print_ntable':
>>> > ipntable.c:435:3: error: format '%llu' expects type 'long long
>>
>> Pushed up a fix for this using casts.
>> Would rather have used inttypes.h but that was incompatible with kernel's
>> __u64.
>
>
> thanks a lot, this fix ipntable.c .
> Now I got this same type build error on tcp_metrics.c
>
> [...]
> gcc -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
> -Wmissing-declarations -Wold-style-definition -O2 -I../include
> -DRESOLVE_HOSTNAMES -DLIBDIR=\"/usr/lib\" -DCONFDIR=\"/etc/iproute2\"
> -D_GNU_SOURCE   -c -o tcp_metrics.o tcp_metrics.c
> cc1: warnings being treated as errors
> tcp_metrics.c: In function 'process_msg':
> tcp_metrics.c:172:4: error: format '%llu' expects type 'long long
> unsigned int', but argument 3 has type '__u64'
> tcp_metrics.c:172:4: error: format '%03llu' expects type 'long long
> unsigned int', but argument 4 has type '__u64'
> tcp_metrics.c:232:4: error: format '%llu' expects type 'long long
> unsigned int', but argument 4 has type '__u64'
> tcp_metrics.c:232:4: error: format '%03llu' expects type 'long long
> unsigned int', but argument 5 has type '__u64'
> make[1]: *** [tcp_metrics.o] Error 1
> make[1]: Leaving directory `/root/iproute2-3.8.0/ip'
> make: *** [all] Error 2
>
>
>
> Nello
> --
> LinuxPPC64 user.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply

* [PATCH v2] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS
From: Guenter Roeck @ 2013-02-27 20:57 UTC (permalink / raw)
  To: netdev
  Cc: linux-sctp, Vlad Yasevich, Sridhar Samudrala, Neil Horman,
	David S. Miller, Guenter Roeck

Building sctp may fail with:

In function ‘copy_from_user’,
    inlined from ‘sctp_getsockopt_assoc_stats’ at
    net/sctp/socket.c:5656:20:
arch/x86/include/asm/uaccess_32.h:211:26: error: call to
    ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
    buffer size is not provably correct

if built with W=1 due to a missing parameter size validation
before the call to copy_from_user.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fix by moving the existing parameter size validation up
    in the function instead of adding an additional one.

 net/sctp/socket.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index cedd9bf..9ef5c73 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5653,6 +5653,9 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
 	if (len < sizeof(sctp_assoc_t))
 		return -EINVAL;
 
+	/* Allow the struct to grow and fill in as much as possible */
+	len = min_t(size_t, len, sizeof(sas));
+
 	if (copy_from_user(&sas, optval, len))
 		return -EFAULT;
 
@@ -5686,9 +5689,6 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
 	/* Mark beginning of a new observation period */
 	asoc->stats.max_obs_rto = asoc->rto_min;
 
-	/* Allow the struct to grow and fill in as much as possible */
-	len = min_t(size_t, len, sizeof(sas));
-
 	if (put_user(len, optlen))
 		return -EFAULT;
 
-- 
1.7.9.7

^ permalink raw reply related

* Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message
From: Vlad Yasevich @ 2013-02-27 20:58 UTC (permalink / raw)
  To: David Miller; +Cc: linux, netdev, linux-sctp, vyasevich, sri, nhorman
In-Reply-To: <20130227.153344.2178498124458359369.davem@davemloft.net>

On 02/27/2013 03:33 PM, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Wed, 27 Feb 2013 11:43:51 -0800
>
>> Building sctp may fail with:
>>
>> In function ‘copy_from_user’,
>>      inlined from ‘sctp_getsockopt_assoc_stats’ at
>>      net/sctp/socket.c:5656:20:
>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>      ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>>      buffer size is not provably correct
>>
>> if built with W=1 due to a missing parameter size validation.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> This change is correct, but please fix this by simply moving the:
>
> 	/* Allow the struct to grow and fill in as much as possible */
> 	len = min_t(size_t, len, sizeof(sas));
>
> line higher up in the function.
>
> And I also prefer this because:
>
> 	something testing sizeof(foo);
> 	if (copy_from_user(..., ..., sizeof(foo)))
>
> must easier to audit and validate, especially in patch form.
>
> Otherwise I have to bring the code into an editor and read the whole
> function just to make sure you got the type correct.

Right.  In this particular case, we are after a very small portion of 
user data (just the association id) and copying the entire structure is
rather pointless.

A nicer solution here would be to do this:
	 if (copy_from_user(&sas, optval, sizeof(sctp_assoc_t))

We are already guaranteed that much space and we make sure that we don't 
overwrite the kernel stack.

-vlad

^ permalink raw reply

* Re: [PATCH v2] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS
From: Vlad Yasevich @ 2013-02-27 21:00 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: netdev, linux-sctp, Sridhar Samudrala, Neil Horman,
	David S. Miller
In-Reply-To: <1361998651-19354-1-git-send-email-linux@roeck-us.net>

On 02/27/2013 03:57 PM, Guenter Roeck wrote:
> Building sctp may fail with:
>
> In function ‘copy_from_user’,
>      inlined from ‘sctp_getsockopt_assoc_stats’ at
>      net/sctp/socket.c:5656:20:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>      ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>      buffer size is not provably correct
>
> if built with W=1 due to a missing parameter size validation
> before the call to copy_from_user.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Fix by moving the existing parameter size validation up
>      in the function instead of adding an additional one.

This works too...

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

>
>   net/sctp/socket.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index cedd9bf..9ef5c73 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5653,6 +5653,9 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>   	if (len < sizeof(sctp_assoc_t))
>   		return -EINVAL;
>
> +	/* Allow the struct to grow and fill in as much as possible */
> +	len = min_t(size_t, len, sizeof(sas));
> +
>   	if (copy_from_user(&sas, optval, len))
>   		return -EFAULT;
>
> @@ -5686,9 +5689,6 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>   	/* Mark beginning of a new observation period */
>   	asoc->stats.max_obs_rto = asoc->rto_min;
>
> -	/* Allow the struct to grow and fill in as much as possible */
> -	len = min_t(size_t, len, sizeof(sas));
> -
>   	if (put_user(len, optlen))
>   		return -EFAULT;
>
>

^ permalink raw reply

* [PATCH net 0/3] bnx2x: Link/LED fixes
From: Yaniv Rosner @ 2013-02-27 23:06 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Yaniv Rosner

Hi Dave,
The following patch series describe couple of link fixes.
Please consider applying it to net.

Thanks,
Yaniv

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox