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] bcm43xx-d80211: return correct hard_start_xmit error code
Hi John, Please apply this to wireless-dev. -- hard_start_xmit should return a NETDEV_TX_FOO error code. Signed-off-by: Michael Buesch <[EMAIL PROTECTED]> Index: wireless-dev/drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c === --- wireless-dev.orig/drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c 2006-08-19 18:26:03.0 +0200 +++ wireless-dev/drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c 2006-08-19 20:11:20.0 +0200 @@ -3980,7 +3980,9 @@ } spin_unlock_irqrestore(&bcm->irq_lock, flags); - return err; + if (unlikely(err)) + return NETDEV_TX_BUSY; + return NETDEV_TX_OK; } static int bcm43xx_net_reset(struct net_device *net_dev) -- 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
[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);
Re: [patch 15/41] ni52 section fix
[EMAIL PROTECTED] wrote: From: Andrew Morton <[EMAIL PROTECTED]> WARNING: drivers/net/ni52.o - Section mismatch: reference to .init.text:ni52_probe from .text between 'init_module' (at offset 0x997) and 'ni52_close' WARNING: drivers/net/ni65.o - Section mismatch: reference to .init.text:ni65_probe from .text between 'init_module' (at offset 0x54a) and 'ni65_stop_start' Cc: Jeff Garzik <[EMAIL PROTECTED]> Signed-off-by: Andrew Morton <[EMAIL PROTECTED]> I'm applying these, but in the future please collapse a series of one-liner changes, all of which apply the same change to different drivers, into just a few patches. Or even just one patch. 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
[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 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 > 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 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 with the non-indented else branches. Learn about goto. > Thanks Alexander ! > Willy > > > > Thanks, > > > > -- > > Alexander Peslyak > > 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 > > - > 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 > -- 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: [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
Re: [PATCH 2/6]: powerpc/cell spidernet low watermark patch.
> card->low_watermark->next->dmac_cmd_status |= SPIDER_NET_DESCR_TXDESFLG; > mb(); > card->low_watermark->dmac_cmd_status &= ~SPIDER_NET_DESCR_TXDESFLG; > card->low_watermark = card->low_watermark->next; > > when we queue another frame for TX. I would have expected those to be racy vs. the hardware... what if the hardware is updating dmac_cmd_status just as your are trying to and the bit out of it ? Ben - 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