Re: [2.6.19 PATCH 4/7] ehea: ethtool interface
On Fri, 2006-08-18 at 17:41 +0200, Thomas Klein wrote: Hi Alexey, first of all thanks a lot for the extensive review. Alexey Dobriyan wrote: + u64 hret = H_HARDWARE; Useless assignment here and everywhere. Initializing returncodes to errorstate is a cheap way to prevent accidentally returning (uninitalized) success returncodes which can lead to catastrophic misbehaviour. If you try to return an uninitialized value the compiler will warn you, you'll then look at the code and realise you missed a case, you might save yourself a bug. By unconditionally initialising you are lying to the compiler, and it can no longer help you. cheers -- Michael Ellerman IBM OzLabs wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part
Re: [2.6.19 PATCH 4/7] ehea: ethtool interface
On Sat, 2006-08-19 at 16:18 +1000, Michael Ellerman wrote: If you try to return an uninitialized value the compiler will warn you, you'll then look at the code and realise you missed a case, you might save yourself a bug. You *should* look at the code :) So should we be reporting these as bugs? [EMAIL PROTECTED]:~/linux/linux-2.6.17.6$ script make.script Script started, file is make.script [EMAIL PROTECTED]:~/linux/linux-2.6.17.6$ make ... Script done, file is make.script [EMAIL PROTECTED]:~/linux/linux-2.6.17.6$ fgrep warning make.script arch/i386/kernel/cpu/transmeta.c:12: warning: 'cpu_freq' may be used uninitialized in this function fs/bio.c:169: warning: 'idx' may be used uninitialized in this function fs/eventpoll.c:500: warning: 'fd' may be used uninitialized in this function fs/isofs/namei.c:162: warning: 'offset' may be used uninitialized in this function fs/isofs/namei.c:162: warning: 'block' may be used uninitialized in this function fs/nfsd/nfsctl.c:292: warning: 'maxsize' may be used uninitialized in this function fs/udf/balloc.c:751: warning: 'goal_eloc.logicalBlockNum' may be used uninitialized in this function fs/udf/super.c:1358: warning: 'ino.partitionReferenceNum' may be used uninitialized in this function fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_startblock' may be used uninitialized in this function fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_blockcount' may be used uninitialized in this function fs/xfs/xfs_bmap.c:2498: warning: 'rtx' is used uninitialized in this function fs/xfs/xfs_bmap_btree.c:753: warning: 'nkey.br_startoff' may be used uninitialized in this function fs/xfs/xfs_da_btree.c:151: warning: 'action' may be used uninitialized in this function fs/xfs/xfs_dir.c:363: warning: 'totallen' may be used uninitialized in this function fs/xfs/xfs_dir.c:363: warning: 'count' may be used uninitialized in this function fs/xfs/xfs_ialloc_btree.c:545: warning: 'nkey.ir_startino' may be used uninitialized in this function fs/xfs/xfs_inode.c:1958: warning: 'last_dip' may be used uninitialized in this function fs/xfs/xfs_inode.c:1960: warning: 'last_offset' may be used uninitialized in this function fs/xfs/xfs_log.c:1749: warning: 'iclog' may be used uninitialized in this function fs/xfs/xfs_log_recover.c:523: warning: 'first_blk' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.qbytes' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.uid' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.gid' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.mode' may be used uninitialized in this function ipc/sem.c:810: warning: 'setbuf.uid' may be used uninitialized in this function ipc/sem.c:810: warning: 'setbuf.gid' may be used uninitialized in this function ipc/sem.c:810: warning: 'setbuf.mode' may be used uninitialized in this function drivers/md/dm-table.c:431: warning: 'dev' may be used uninitialized in this function drivers/md/dm-ioctl.c:1388: warning: 'param' may be used uninitialized in this function net/sched/sch_cbq.c:409: warning: 'ret' may be used uninitialized in this function lib/zlib_inflate/inftrees.c:121: warning: 'r.base' may be used uninitialized in this function - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Network receive stall avoidance (was [PATCH 2/9] deadlock prevention core)
On Fri, 18 Aug 2006 21:14:09 -0700 Daniel Phillips [EMAIL PROTECTED] wrote: So rather than just the word deadlock, let us add or atomic 0 order alloc failure during TCP receive to the challenge. Fair? If it's significantly performance-affecting in any way which is at all likely to affect anyone, sure. You can get those warnings now with regular networking using e1000, due to a combination of excessive default rx ringsize and incorrect VM tuning. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6.19 PATCH 4/7] ehea: ethtool interface
On Sat, 2006-08-19 at 02:48 -0400, Andy Gay wrote:
On Sat, 2006-08-19 at 16:18 +1000, Michael Ellerman wrote:
If you try to return an uninitialized value the compiler will warn you,
you'll then look at the code and realise you missed a case, you might
save yourself a bug.
You *should* look at the code :)
So should we be reporting these as bugs?
No you're better off sending patches ;)
A lot of these have started appearing recently, which I think is due to
GCC becoming more vocal. Unfortunately many of them are false positives
caused by GCC not seeming to grok that this is ok:
void foo(int *x) { *x = 1; }
...
int x;
foo(x);
return x;
It's a pity because it creates noise, but still it's beside the point.
New code going into the kernel should be 100% warning free, and so if
the eHEA guys had missed an error case they'd spot the warning before
they submitted it.
Doing the initialise-to-some-value trick means you only spot the bug
via testing.
cheers
--
Michael Ellerman
IBM OzLabs
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 5/7] ip1000: Modify coding style of ipg_config_autoneg()
Jesse Huang [EMAIL PROTECTED] : From: Jesse Huang [EMAIL PROTECTED] This is only coding style modify for ipg_config_autoneg(). Thanks for the suggestion form Francois. Change Logs: Modify coding style of ipg_config_autoneg() --- drivers/net/ipg.c | 17 ++--- 1 files changed, 10 insertions(+), 7 deletions(-) 737498ca620437d8179e21be4d5220333066cbbd diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c index f859107..be96f93 100644 --- a/drivers/net/ipg.c +++ b/drivers/net/ipg.c @@ -491,11 +491,13 @@ static int ipg_config_autoneg(struct net int fullduplex; int txflowcontrol; int rxflowcontrol; + long MacCtrlValue; Mixed case variables are not exactly welcome. -- Ueimor - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
wireless: NETDEV_TX_LOCKED, use it or not?
Hi, I am currently wondering, if we should use NETDEV_TX_LOCKED in the bcm43xx wireless net driver. I currently don't have an SMP machine with a bcm43xx card running, so I can't use benchmarks to find out. There are various things to consider: * The RX and TX-status paths, which also must take the spinlock aren't more expensive than on a normal Ethernet card (at least with d80211 stack). * But the retry-path, when a NETDEV_TX_LOCKED is returned is slightly more expensive. Well, the question is: Is it worth to return NETDEV_TX_LOCKED on spinlock contention, or is it better to spin? Is NETDEV_TX_LOCKED only desired on gigabit ethernet? Thanks. -- Greetings Michael. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] HOWTO use NAPI to reduce TX interrupts
On Sunday 20 August 2006 03:31, Stephen Hemminger wrote: The reason reclaim via poll() is efficient is because it avoid causing a softirq that is necessary when skb_free_irq() is done. Instead it reuses the softirq from the poll() routine. Ok, I completely missed this point so far, thanks for the info. Like all Rx NAPI, using poll() for reclaim means: + aggregating multiple frames in one irq - increased overhead of twiddling with the IRQ mask - more ways to get driver stuck What is the best way to treat the IRQ mask for TX interrupts? I guess it should be roughly: - off when we expect -poll() to be called, i.e. after calling netif_rx_schedule() or returning after a partial rx from poll(). - off when there are no packets left in the TX queue - on while RX interrupts are on and we're waiting for packets to be transmitted. Some drivers do all their irq work in the poll() routine (including PHY handling). This is good if reading the IRQ status does an auto mask operation. The whole NAPI documentation area is a mess and needs a good writer to do some major restructuring. It should also be split into reference information, tutorial and guide sections. I won't be able to do that work, I'm neither a good writer nor a networking person. Do you think we should still merge a section like the text I wrote up, even if it makes the text even less well structured? Should I maybe add it somewhere else than the appendix? Arnd - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6.19 PATCH 4/7] ehea: ethtool interface
On Saturday 19 August 2006 10:41, Michael Ellerman wrote:
A lot of these have started appearing recently, which I think is due to
GCC becoming more vocal. Unfortunately many of them are false positives
caused by GCC not seeming to grok that this is ok:
void foo(int *x) { *x = 1; }
...
int x;
foo(x);
return x;
It's more subtle than this, gcc only gets it wrong when multiple
things come together, the most common one seems to be:
- it tries to inline foo()
- foo has a path where it initializes *x and another one where it
doesn't
- x is accessed after foo() returns, but only when foo indeed has
initialized it.
The problem is that gcc now is more aggressive about inlining
functions. It used to assume that all functions initialize their
pointer arguments, now it does some more checking, but not enough,
so there are lots of false positives. Every gcc-4.x release seems
to fix some of these cases, but a few others remain.
Arnd
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [2.6.19 PATCH 4/7] ehea: ethtool interface
Andy Gay wrote: fs/bio.c:169: warning: 'idx' may be used uninitialized in this function fs/eventpoll.c:500: warning: 'fd' may be used uninitialized in this function fs/isofs/namei.c:162: warning: 'offset' may be used uninitialized in this function fs/isofs/namei.c:162: warning: 'block' may be used uninitialized in this function fs/nfsd/nfsctl.c:292: warning: 'maxsize' may be used uninitialized in this function fs/udf/balloc.c:751: warning: 'goal_eloc.logicalBlockNum' may be used uninitialized in this function fs/udf/super.c:1358: warning: 'ino.partitionReferenceNum' may be used uninitialized in this function fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_startblock' may be used uninitialized in this function fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_blockcount' may be used uninitialized in this function fs/xfs/xfs_bmap.c:2498: warning: 'rtx' is used uninitialized in this function fs/xfs/xfs_bmap_btree.c:753: warning: 'nkey.br_startoff' may be used uninitialized in this function fs/xfs/xfs_da_btree.c:151: warning: 'action' may be used uninitialized in this function fs/xfs/xfs_dir.c:363: warning: 'totallen' may be used uninitialized in this function fs/xfs/xfs_dir.c:363: warning: 'count' may be used uninitialized in this function fs/xfs/xfs_ialloc_btree.c:545: warning: 'nkey.ir_startino' may be used uninitialized in this function fs/xfs/xfs_inode.c:1958: warning: 'last_dip' may be used uninitialized in this function fs/xfs/xfs_inode.c:1960: warning: 'last_offset' may be used uninitialized in this function fs/xfs/xfs_log.c:1749: warning: 'iclog' may be used uninitialized in this function fs/xfs/xfs_log_recover.c:523: warning: 'first_blk' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.qbytes' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.uid' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.gid' may be used uninitialized in this function ipc/msg.c:338: warning: 'setbuf.mode' may be used uninitialized in this function ipc/sem.c:810: warning: 'setbuf.uid' may be used uninitialized in this function ipc/sem.c:810: warning: 'setbuf.gid' may be used uninitialized in this function ipc/sem.c:810: warning: 'setbuf.mode' may be used uninitialized in this function drivers/md/dm-table.c:431: warning: 'dev' may be used uninitialized in this function drivers/md/dm-ioctl.c:1388: warning: 'param' may be used uninitialized in this function net/sched/sch_cbq.c:409: warning: 'ret' may be used uninitialized in this function lib/zlib_inflate/inftrees.c:121: warning: 'r.base' may be used uninitialized in this function These are gcc bugs. We don't patch the kernel for gcc bugs. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 2/9] deadlock prevention core
Andrew Morton wrote: - We expect that the lots-of-dirty-anon-memory-over-swap-over-network scenario might still cause deadlocks. I assert that this can be solved by putting swap on local disks. Peter asserts that this isn't acceptable due to disk unreliability. I point out that local disk reliability can be increased via MD, all goes quiet. A good exposition which helps us to understand whether and why a significant proportion of the target user base still wishes to do swap-over-network would be useful. You cannot put disks in many models of blade servers. At all. -- What is important? What you want to be true, or what is true? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bcm43xx: return correct hard_start_xmit error code]
John, Please apply the following patch by Michael Buesch to wireless-2.6. Larry == hard_start_xmit should return a NETIF_TX_FOO error code. Signed-off-by: Michael Buesch [EMAIL PROTECTED] Signed-Off-By: Larry Finger [EMAIL PROTECTED] Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c === --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.c +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c @@ -3918,7 +3918,9 @@ err = bcm43xx_tx(bcm, txb); spin_unlock_irqrestore(bcm-irq_lock, flags); - return err; + if (unlikely(err)) + return NETDEV_TX_BUSY; + return NETDEV_TX_OK; } static struct net_device_stats * bcm43xx_net_get_stats(struct net_device *net_dev) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 2/9] deadlock prevention core
On 8/18/06, Andrew Morton [EMAIL PROTECTED] wrote: I assert that this can be solved by putting swap on local disks. Peter asserts that this isn't acceptable due to disk unreliability. I point out that local disk reliability can be increased via MD, all goes quiet. A good exposition which helps us to understand whether and why a significant proportion of the target user base still wishes to do swap-over-network would be useful. Adding a hard drive adds $low per system, another failure point, and more importantly ~3-10 Watts which then has to be paid for twice (once to power it, again to cool it). For a hundred seats, that's significant. For 500, it's ranging toward fully painful. I'm in the process of designing the next upgrade for a VoIP call center, and we want to go entirely diskless in the agent systems. We'd also rather not swap over the network, but 'swap is as swap does.' That said, it in no way invalidates using /proc/sys/vm/min_free_kbytes... Ray - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH wireless-dev] d80211: Group EIDs by standard, add remaining 802.11d EIDs
d80211: Group EIDs by standard, add remaining 802.11d EIDs
This patch groups EIDs together by the 802.11 standard they were introduced in
and adds the remaining 802.11d EIDs. The spec where the QoS EID was
introduced still needs to be found. (does not appear to be 802.11e..) This
patch depends on the previous patch that converts the status/reason codes and
EIDs to enums.
Signed-off-by: Michael Wu [EMAIL PROTECTED]
---
include/net/d80211_mgmt.h | 13 +
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/include/net/d80211_mgmt.h b/include/net/d80211_mgmt.h
index 6981c6f..eb032de 100644
--- a/include/net/d80211_mgmt.h
+++ b/include/net/d80211_mgmt.h
@@ -192,9 +192,13 @@ enum ieee80211_eid {
WLAN_EID_CF_PARAMS = 4,
WLAN_EID_TIM = 5,
WLAN_EID_IBSS_PARAMS = 6,
- WLAN_EID_COUNTRY = 7,
WLAN_EID_CHALLENGE = 16,
-/* EIDs defined as part fo 11h - starts */
+ /* 802.11d */
+ WLAN_EID_COUNTRY = 7,
+ WLAN_EID_HP_PARAMS = 8,
+ WLAN_EID_HP_TABLE = 9,
+ WLAN_EID_REQUEST = 10,
+ /* 802.11h */
WLAN_EID_PWR_CONSTRAINT = 32,
WLAN_EID_PWR_CAPABILITY = 33,
WLAN_EID_TPC_REQUEST = 34,
@@ -205,10 +209,11 @@ enum ieee80211_eid {
WLAN_EID_MEASURE_REPORT = 39,
WLAN_EID_QUIET = 40,
WLAN_EID_IBSS_DFS = 41,
-/* EIDs defined as part fo 11h - ends */
+ /* 802.11g */
WLAN_EID_ERP_INFO = 42,
- WLAN_EID_RSN = 48,
WLAN_EID_EXT_SUPP_RATES = 50,
+ /* 802.11i */
+ WLAN_EID_RSN = 48,
WLAN_EID_WPA = 221,
WLAN_EID_GENERIC = 221,
WLAN_EID_VENDOR_SPECIFIC = 221,
pgptcuDzypGLW.pgp
Description: PGP signature
Re: [PATCH] e1000: ring buffers resources cleanup
Hello Joe,
Joe Perches wrote:
On Fri, 2006-08-18 at 19:02 +0400, Vasily Averin wrote:
Memory leak was found in 2.6.18-rc4 and e1000 7.2.7 from sourceforge:
We should free resources allocated for previous rings if following allocation
fails.
Did you read the comment headers in the function?
* If this function returns with an error, then it's possible one or
* more of the rings is populated (while the rest are not). It is the
* callers duty to clean those orphaned rings.
Thank you for your notice.
I believe this comment is incorrect: if some function returns an error it should
restore original state on exit, otherwise can lead to resource leaks. Also I
would note that this requirements is not accomplished in current driver version:
e1000_setup_all_Xx_resources functions are called in two places: in
e1000_set_ringparam() and in e1000_open() and in both cases nobody cleans those
orphaned rings.
Therefore I think it make sense to remove these comments too.
Andrew, could you please use attached patch instead previous version?
---
Memory leak was found in 2.6.18-rc4 and e1000 7.2.7 from sourceforge:
We should free resources allocated for previous rings if following allocation
fails. Also incorrect comments in e1000_setup_all_Xx_resources() are removed
Signed-off-by: Vasily Averin [EMAIL PROTECTED]
Thank you,
Vasily Averin
SWsoft Virtuozzo/OpenVZ Linux kernel team
--- linux-2.6.18-rc4/drivers/net/e1000/e1000_main.c.irsrs 2006-08-18
16:58:51.0 +0400
+++ linux-2.6.18-rc4/drivers/net/e1000/e1000_main.c 2006-08-19
22:54:00.0 +0400
@@ -1381,10 +1381,6 @@ setup_tx_desc_die:
* (Descriptors) for all queues
* @adapter: board private structure
*
- * If this function returns with an error, then it's possible one or
- * more of the rings is populated (while the rest are not). It is the
- * callers duty to clean those orphaned rings.
- *
* Return 0 on success, negative on failure
**/
@@ -1398,6 +1394,9 @@ e1000_setup_all_tx_resources(struct e100
if (err) {
DPRINTK(PROBE, ERR,
Allocation for Tx Queue %u failed\n, i);
+ for (i-- ; i = 0; i--)
+ e1000_free_tx_resources(adapter,
+ adapter-tx_ring[i]);
break;
}
}
@@ -1639,10 +1638,6 @@ setup_rx_desc_die:
* (Descriptors) for all queues
* @adapter: board private structure
*
- * If this function returns with an error, then it's possible one or
- * more of the rings is populated (while the rest are not). It is the
- * callers duty to clean those orphaned rings.
- *
* Return 0 on success, negative on failure
**/
@@ -1656,6 +1651,9 @@ e1000_setup_all_rx_resources(struct e100
if (err) {
DPRINTK(PROBE, ERR,
Allocation for Rx Queue %u failed\n, i);
+ for (i-- ; i = 0; i--)
+ e1000_free_rx_resources(adapter,
+ adapter-rx_ring[i]);
break;
}
}
[PATCH] e1000: IRQ resource cleanup
patch updated: It seems for me e1000_power_down_phy() should be added to rollbacke1000_power_up_phy(). I know, currently e1000_up() always return success. But this behaviour may be changed in future... Andrew, could you please use attached patch instead previous version? --- irq leak was found in 2.6.18-rc4 and e1000 7.2.7 from sourceforge: if e1000_up fails in e1000_open() we do not free allocated irq Signed-off-by: Vasily Averin [EMAIL PROTECTED] Thank you, Vasily Averin SWsoft Virtuozzo/OpenVZ Linux kernel team --- linux-2.6.18-rc4/drivers/net/e1000/e1000_main.c.oirq1 2006-08-19 22:57:02.0 +0400 +++ linux-2.6.18-rc4/drivers/net/e1000/e1000_main.c 2006-08-19 23:02:45.0 +0400 @@ -1208,7 +1208,7 @@ e1000_open(struct net_device *netdev) err = e1000_request_irq(adapter); if (err) - goto err_up; + goto err_req_irq; e1000_power_up_phy(adapter); @@ -1229,6 +1229,9 @@ e1000_open(struct net_device *netdev) return E1000_SUCCESS; err_up: + e1000_power_down_phy(adapter); + e1000_free_irq(adapter); +err_req_irq: e1000_free_all_rx_resources(adapter); err_setup_rx: e1000_free_all_tx_resources(adapter);
[PATCH] getsockopt() early argument sanity checking
Willy,
I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
into 2.4.34-pre.
(2.6 kernels could benefit from the same change, too, but at the moment
I am dealing with proper submission of generic changes like this that
are a part of 2.4.33-ow1.)
The patch makes getsockopt(2) sanity-check the value pointed to by
the optlen argument early on. This is a security hardening measure
intended to prevent exploitation of certain potential vulnerabilities in
socket type specific getsockopt() code on UP systems.
This change has been a part of -ow patches for some years.
Thanks,
--
Alexander Peslyak solar at openwall.com
GPG key ID: B35D3598 fp: 6429 0D7E F130 C13E C929 6447 73C3 A290 B35D 3598
http://www.openwall.com - bringing security into open computing environments
diff -urpPX nopatch linux-2.4.33/net/socket.c linux/net/socket.c
--- linux-2.4.33/net/socket.c Wed Jan 19 17:10:14 2005
+++ linux/net/socket.c Sat Aug 12 08:51:47 2006
@@ -1307,10 +1307,18 @@ asmlinkage long sys_setsockopt(int fd, i
asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval,
int *optlen)
{
int err;
+ int len;
struct socket *sock;
if ((sock = sockfd_lookup(fd, err))!=NULL)
{
+ /* XXX: insufficient for SMP, but should be redundant anyway */
+ if (get_user(len, optlen))
+ err = -EFAULT;
+ else
+ if (len 0)
+ err = -EINVAL;
+ else
if (level == SOL_SOCKET)
err=sock_getsockopt(sock,level,optname,optval,optlen);
else
Re: [PATCH] getsockopt() early argument sanity checking
On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
Willy,
I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
into 2.4.34-pre.
(2.6 kernels could benefit from the same change, too, but at the moment
I am dealing with proper submission of generic changes like this that
are a part of 2.4.33-ow1.)
The patch makes getsockopt(2) sanity-check the value pointed to by
the optlen argument early on. This is a security hardening measure
intended to prevent exploitation of certain potential vulnerabilities in
socket type specific getsockopt() code on UP systems.
This change has been a part of -ow patches for some years.
looks valid to me, merged.
Thanks Alexander !
Willy
Thanks,
--
Alexander Peslyak solar at openwall.com
GPG key ID: B35D3598 fp: 6429 0D7E F130 C13E C929 6447 73C3 A290 B35D 3598
http://www.openwall.com - bringing security into open computing environments
diff -urpPX nopatch linux-2.4.33/net/socket.c linux/net/socket.c
--- linux-2.4.33/net/socket.c Wed Jan 19 17:10:14 2005
+++ linux/net/socket.cSat Aug 12 08:51:47 2006
@@ -1307,10 +1307,18 @@ asmlinkage long sys_setsockopt(int fd, i
asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval,
int *optlen)
{
int err;
+ int len;
struct socket *sock;
if ((sock = sockfd_lookup(fd, err))!=NULL)
{
+ /* XXX: insufficient for SMP, but should be redundant anyway */
+ if (get_user(len, optlen))
+ err = -EFAULT;
+ else
+ if (len 0)
+ err = -EINVAL;
+ else
if (level == SOL_SOCKET)
err=sock_getsockopt(sock,level,optname,optval,optlen);
else
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] getsockopt() early argument sanity checking
On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
On Sunday 20 August 2006 01:48, Willy Tarreau wrote:
On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
Willy,
I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
into 2.4.34-pre.
(2.6 kernels could benefit from the same change, too, but at the moment
I am dealing with proper submission of generic changes like this that
are a part of 2.4.33-ow1.)
The patch makes getsockopt(2) sanity-check the value pointed to by
the optlen argument early on. This is a security hardening measure
intended to prevent exploitation of certain potential vulnerabilities in
socket type specific getsockopt() code on UP systems.
This change has been a part of -ow patches for some years.
looks valid to me, merged.
Not to me. It heavily violates codingstyle and screws brains
^^^
little exageration detected here.
with the non-indented else branches.
while they surprized me first, they make the *patch* more readable
by clearly showing what has been inserted and where. However, I have
joined the lines for the merge.
Learn about goto.
definitely not here. The if() expressions are all one-liners. Adding
a goto would mean two instructions, to which you add 2 braces. It will
not make the code more readable. Patch below is OK. If you have a hard
time understanding it, then it's because it's bedtime for you too :-)
Regards,
Willy
diff --git a/net/socket.c b/net/socket.c
index ac45b13..910ef88 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1307,11 +1307,17 @@ asmlinkage long sys_setsockopt(int fd, i
asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval,
int *optlen)
{
int err;
+ int len;
struct socket *sock;
if ((sock = sockfd_lookup(fd, err))!=NULL)
{
- if (level == SOL_SOCKET)
+ /* XXX: insufficient for SMP, but should be redundant anyway */
+ if (get_user(len, optlen))
+ err = -EFAULT;
+ else if (len 0)
+ err = -EINVAL;
+ else if (level == SOL_SOCKET)
err=sock_getsockopt(sock,level,optname,optval,optlen);
else
err=sock-ops-getsockopt(sock, level, optname, optval,
optlen);
--
1.4.1
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 2/9] deadlock prevention core
Rik van Riel wrote: Andrew Morton wrote: - We expect that the lots-of-dirty-anon-memory-over-swap-over-network scenario might still cause deadlocks. I assert that this can be solved by putting swap on local disks. Peter asserts that this isn't acceptable due to disk unreliability. I point out that local disk reliability can be increased via MD, all goes quiet. A good exposition which helps us to understand whether and why a significant proportion of the target user base still wishes to do swap-over-network would be useful. You cannot put disks in many models of blade servers. At all. Or many thin clients in general. They are used in quite a few schools over here, running Linux. Some of them do in fact have space for disks, but disks adds costs (heat, power, replacing failed drives) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Proper pci_enable_device() error handling in resume routine
Hello, I'm trying to properly handle pci_enable_device() errors in the resume routines of a couple of tulip drivers. I noticed that several drivers pay attention to errors from pci_enable_device() in the init routine but ignore it on resume; other drivers vary wildly. What's proper behavior when resuming? Extant examples: 0. Don't call pci_enable_device() at all (8139too) 1. Ignore the return value (eepro100, many others) 2. Check for failure and bail out, but return success (sungem) -VAL - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
