* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Tabi Timur-B04825 @ 2012-08-20 0:59 UTC (permalink / raw)
To: Francois Romieu
Cc: Fleming Andy-AFLEMING, David Miller, netdev@vger.kernel.org
In-Reply-To: <20120819224804.GA28300@electric-eye.fr.zoreil.com>
On Aug 19, 2012, at 6:03 PM, "Francois Romieu" <romieu@fr.zoreil.com> wrote:
> Tabi Timur-B04825 <B04825@freescale.com> :
> [...]
>> I don't see any real savings here.
>
> xgmac_mdio_free_bus_wait could return -ETIMEDOUT and include the
> debug message.
Ok.
>
> --
> Ueimor
>
^ permalink raw reply
* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Francois Romieu @ 2012-08-19 22:48 UTC (permalink / raw)
To: Tabi Timur-B04825
Cc: Fleming Andy-AFLEMING, David Miller, netdev@vger.kernel.org
In-Reply-To: <50315A18.2040105@freescale.com>
Tabi Timur-B04825 <B04825@freescale.com> :
[...]
> I don't see any real savings here.
xgmac_mdio_free_bus_wait could return -ETIMEDOUT and include the
debug message.
--
Ueimor
^ permalink raw reply
* [PATCH v2] serial: add a new helper function
From: Huang Shijie @ 2012-08-19 21:33 UTC (permalink / raw)
To: gregkh; +Cc: alan, jirislaby, linux-kernel, linux-serial, netdev, Huang Shijie
In most of the time, the driver needs to check if the cts flow control
is enabled. But now, the driver checks the ASYNC_CTS_FLOW flag manually,
which is not a grace way. So add a new wraper function to make the code
tidy and clean.
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
v1 --> v2:
move the new help function to serial.h
---
drivers/char/pcmcia/synclink_cs.c | 2 +-
drivers/tty/amiserial.c | 2 +-
drivers/tty/cyclades.c | 2 +-
drivers/tty/isicom.c | 2 +-
drivers/tty/mxser.c | 2 +-
drivers/tty/serial/mxs-auart.c | 2 +-
drivers/tty/serial/serial_core.c | 4 ++--
drivers/tty/synclink.c | 2 +-
drivers/tty/synclink_gt.c | 2 +-
drivers/tty/synclinkmp.c | 2 +-
include/linux/serial.h | 7 +++++++
net/irda/ircomm/ircomm_tty.c | 4 ++--
net/irda/ircomm/ircomm_tty_attach.c | 2 +-
13 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 5db08c7..3f57d5de 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -1050,7 +1050,7 @@ static void cts_change(MGSLPC_INFO *info, struct tty_struct *tty)
wake_up_interruptible(&info->status_event_wait_q);
wake_up_interruptible(&info->event_wait_q);
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (info->serial_signals & SerialSignal_CTS) {
if (debug_level >= DEBUG_LEVEL_ISR)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 2b7535d..42d0a25 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -420,7 +420,7 @@ static void check_modem_status(struct serial_state *info)
tty_hangup(port->tty);
}
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (port->tty->hw_stopped) {
if (!(status & SER_CTS)) {
#if (defined(SERIAL_DEBUG_INTR) || defined(SERIAL_DEBUG_FLOW))
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index e3954da..0a6a0bc 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -727,7 +727,7 @@ static void cyy_chip_modem(struct cyclades_card *cinfo, int chip,
else
tty_hangup(tty);
}
- if ((mdm_change & CyCTS) && (info->port.flags & ASYNC_CTS_FLOW)) {
+ if ((mdm_change & CyCTS) && tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (mdm_status & CyCTS) {
/* cy_start isn't used
diff --git a/drivers/tty/isicom.c b/drivers/tty/isicom.c
index 99cf22e..d7492e1 100644
--- a/drivers/tty/isicom.c
+++ b/drivers/tty/isicom.c
@@ -600,7 +600,7 @@ static irqreturn_t isicom_interrupt(int irq, void *dev_id)
port->status &= ~ISI_DCD;
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (header & ISI_CTS) {
port->port.tty->hw_stopped = 0;
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index bb2da4c..cfda47d 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -830,7 +830,7 @@ static void mxser_check_modem_status(struct tty_struct *tty,
wake_up_interruptible(&port->port.open_wait);
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (status & UART_MSR_CTS) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 3a667ee..dafeef2 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -262,7 +262,7 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
ctrl &= ~AUART_CTRL2_RTSEN;
if (mctrl & TIOCM_RTS) {
- if (u->state->port.flags & ASYNC_CTS_FLOW)
+ if (tty_port_cts_enabled(&u->state->port))
ctrl |= AUART_CTRL2_RTSEN;
}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5b308c8..1920e5c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -176,7 +176,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
spin_lock_irq(&uport->lock);
if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
tty->hw_stopped = 1;
@@ -2493,7 +2493,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
uport->icount.cts++;
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (tty->hw_stopped) {
if (status) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index 666aa14..70e3a52 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -1359,7 +1359,7 @@ static void mgsl_isr_io_pin( struct mgsl_struct *info )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if (info->port.tty->hw_stopped) {
if (status & MISCSTATUS_CTS) {
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 45f6136..b38e954 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -2053,7 +2053,7 @@ static void cts_change(struct slgt_info *info, unsigned short status)
wake_up_interruptible(&info->event_wait_q);
info->pending_bh |= BH_STATUS;
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (info->port.tty) {
if (info->port.tty->hw_stopped) {
if (info->signals & SerialSignal_CTS) {
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index 53429c8..f17d9f3 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -2500,7 +2500,7 @@ static void isr_io_pin( SLMP_INFO *info, u16 status )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if ( info->port.tty ) {
if (info->port.tty->hw_stopped) {
diff --git a/include/linux/serial.h b/include/linux/serial.h
index 90e9f98..154dc94 100644
--- a/include/linux/serial.h
+++ b/include/linux/serial.h
@@ -11,6 +11,7 @@
#define _LINUX_SERIAL_H
#include <linux/types.h>
+#include <linux/tty.h>
#ifdef __KERNEL__
#include <asm/page.h>
@@ -218,6 +219,12 @@ struct serial_rs485 {
are a royal PITA .. */
};
+/* If the cts flow control is enabled, return true. */
+static inline bool tty_port_cts_enabled(struct tty_port *port)
+{
+ return port->flags & ASYNC_CTS_FLOW;
+}
+
#ifdef __KERNEL__
#include <linux/compiler.h>
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9668990..95a3a7a 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -1070,7 +1070,7 @@ void ircomm_tty_check_modem_status(struct ircomm_tty_cb *self)
goto put;
}
}
- if (tty && self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty && tty_port_cts_enabled(&self->port)) {
if (tty->hw_stopped) {
if (status & IRCOMM_CTS) {
IRDA_DEBUG(2,
@@ -1313,7 +1313,7 @@ static void ircomm_tty_line_info(struct ircomm_tty_cb *self, struct seq_file *m)
seq_puts(m, "Flags:");
sep = ' ';
- if (self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&self->port)) {
seq_printf(m, "%cASYNC_CTS_FLOW", sep);
sep = '|';
}
diff --git a/net/irda/ircomm/ircomm_tty_attach.c b/net/irda/ircomm/ircomm_tty_attach.c
index 3ab70e7..edab393 100644
--- a/net/irda/ircomm/ircomm_tty_attach.c
+++ b/net/irda/ircomm/ircomm_tty_attach.c
@@ -578,7 +578,7 @@ void ircomm_tty_link_established(struct ircomm_tty_cb *self)
* will have to wait for the peer device (DCE) to raise the CTS
* line.
*/
- if ((self->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&self->port) &&
((self->settings.dce & IRCOMM_CTS) == 0)) {
IRDA_DEBUG(0, "%s(), waiting for CTS ...\n", __func__ );
goto put;
--
1.7.4.4
^ permalink raw reply related
* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Tabi Timur-B04825 @ 2012-08-19 21:26 UTC (permalink / raw)
To: Francois Romieu
Cc: Tabi Timur-B04825, Fleming Andy-AFLEMING, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20120819205724.GA27555@electric-eye.fr.zoreil.com>
Francois Romieu wrote:
>> >+ /* Wait till the bus is free */
>> >+ status = spin_event_timeout(
>> >+ !((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY), TIMEOUT, 0);
>> >+ if (!status) {
>> >+ dev_dbg(&bus->dev, "%s: timeout waiting for stat\n", __func__);
>> >+ return -ETIMEDOUT;
>> >+ }
> This code - comment included - is repeated several times. You may
> consider factoring it out in some xgmac_mdio_free_bus_wait function.
But it's just one line and a test. The function would look like this:
bool xgmac_mdio_free_bus_wait(struct tgec_mdio_controller __iomem *regs)
{
uint32_t status;
status = spin_event_timeout(
!((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY), TIMEOUT, 0);
return status != 0;
}
and then
if (!xgmac_mdio_free_bus_wait(regs)) {
dev_dbg(&bus->dev, "%s: timeout waiting for stat\n", __func__);
return -ETIMEDOUT;
}
I don't see any real savings here.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 2/2] [RFC] netlink: fix possible spoofing from non-root processes
From: Pablo Neira Ayuso @ 2012-08-19 21:23 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <1345224149-5946-3-git-send-email-pablo@netfilter.org>
[-- Attachment #1: Type: text/plain, Size: 550 bytes --]
On Fri, Aug 17, 2012 at 07:22:29PM +0200, pablo@netfilter.org wrote:
[...]
> [ I don't know any FOSS program making use of Netlink to communicate
> to processes, please, let me know if I'm missing anyone important ]
Patrick pinged me for little reminder on NETLINK_USERSOCK. We still
have to allow netlink-to-netlink userspace communication for it.
So, please find a new version of this patch that allows non-root
processes for that Netlink bus. For others, my patch restricts to root
processes the ability of sending messages with dst_pid != 0.
[-- Attachment #2: 0001-RFC-netlink-fix-possible-spoofing-from-non-root-proc.patch --]
[-- Type: text/x-diff, Size: 3095 bytes --]
>From 78ed359b9802569caebbf2d3507d08d6c7204a84 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 16 Aug 2012 17:58:18 +0200
Subject: [PATCH] [RFC] netlink: fix possible spoofing from non-root processes
Non-root user-space processes can send netlink messages to other
processes that are well-known for being subscribed to Netlink
asynchronous notifications. This allows ilegitimate non-root
process to send forged messages to them.
This is usually fixed by checking for Netlink portID in the
message receival path of the user-space process. In general,
portID == 0 means that the origin of the messages comes from the
kernel. Thus, discarding any message not coming from the kernel.
This is true for rtnetlink.
However, ctnetlink sets the portID in event messages that has
been triggered by some user-space process, eg. conntrack utility.
So other processes subscribed to ctnetlink events, eg. conntrackd,
know that the event was triggered by some user-space action.
This patch adds capability validation in case that dst_pid is set
in netlink_sendmsg(). This approach is aggressive since any existing
application using any of the Netlink busses to deliver messages
between two user-space processes will break.
[ Patrick McHardy has pinged me to let me know about NETLINK_USERSOCK.
Thus, this patch sets NL_CFG_F_NONROOT_SEND to allow non-root
netlink-to-netlink userspace communication for that Netlink bus. ]
However, if we want to ensure full backward compatibility, a new
version of this patch including NL_CFG_F_NONROOT_SEND flags need
to be set in all kernel subsystems. However, I don't think it
makes sense to use NETLINK_ROUTE to communicate two processes
that are sending no matter what information that is not related
to link/neighbouring/routing?
Still, if someone wants to make use of Netlink for this, eg.
I remember people willing to implement D-BUS over Netlink,
then they can reserve some Netlink bus explicitly for this and
set NL_CFG_F_NONROOT_SEND to it.
Reported-by: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netlink/af_netlink.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index d04f923..b3e0e2c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1373,7 +1373,8 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
dst_pid = addr->nl_pid;
dst_group = ffs(addr->nl_groups);
err = -EPERM;
- if (dst_group && !netlink_capable(sock, NL_CFG_F_NONROOT_SEND))
+ if ((dst_group || dst_pid) &&
+ !netlink_capable(sock, NL_CFG_F_NONROOT_SEND))
goto out;
} else {
dst_pid = nlk->dst_pid;
@@ -2141,6 +2142,7 @@ static void __init netlink_add_usersock_entry(void)
rcu_assign_pointer(nl_table[NETLINK_USERSOCK].listeners, listeners);
nl_table[NETLINK_USERSOCK].module = THIS_MODULE;
nl_table[NETLINK_USERSOCK].registered = 1;
+ nl_table[NETLINK_USERSOCK].flags = NL_CFG_F_NONROOT_SEND;
netlink_table_ungrab();
}
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Francois Romieu @ 2012-08-19 20:57 UTC (permalink / raw)
To: Timur Tabi; +Cc: Andy Fleming, David Miller, netdev
In-Reply-To: <1345233792-14985-1-git-send-email-timur@freescale.com>
Timur Tabi <timur@freescale.com> :
> Similar to fsl_pq_mdio.c, this driver is for the 10G MDIO controller on
> Freescale Frame Manager Ethernet controllers.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
[...]
> diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
> new file mode 100644
> index 0000000..1894476
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
[...]
> +static int xgmac_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value)
> +{
> + struct tgec_mdio_controller __iomem *regs = bus->priv;
> + uint16_t dev_addr = regnum >> 16;
> + uint32_t status;
> +
> + /* Setup the MII Mgmt clock speed */
> + out_be32(®s->mdio_stat, MDIO_STAT_CLKDIV(100));
> +
> + /* Wait till the bus is free */
> + status = spin_event_timeout(
> + !((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY), TIMEOUT, 0);
> + if (!status) {
> + dev_dbg(&bus->dev, "%s: timeout waiting for stat\n", __func__);
> + return -ETIMEDOUT;
> + }
This code - comment included - is repeated several times. You may
consider factoring it out in some xgmac_mdio_free_bus_wait function.
--
Ueimor
^ permalink raw reply
* Re: network packet corruption in v3.6.0-rc1 (and also in v3.5)
From: Andrew Worsley @ 2012-08-19 20:20 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Alexey Kuznetsov, James Morris, netdev
In-Reply-To: <1345374134.5158.103.camel@edumazet-glaptop>
> OK, this seems to be related to r8712u, still in staging tree.
>
> At first glance, its using a buggy skb_clone(), or kind of a 'reuse skb'
> trick I have no time to investigate.
>
> I suspect that some memory area are overwritten by this driver, and this
> was not noticed with older kernels. With new kernels, its triggering a
> WARN_ON() in tcp stack.
>
> Any chance you can try to reproduce the bug with another adapter ?
Unfortunately I don't have another USB dongle. I could try to get the built in
Broadcom Corporation BCM4331 802.11a/b/g/n (rev 02)
Subsystem: Apple Inc. Device 00ef
but I need to dig out the proprietary firmware - which will take some
time as I am not familiar with the procedure.
The weird thing about this USB dongle is that I brought it because it
was claimed to be supported under Linux
idVendor 0x0b05 ASUSTek Computer, Inc.
idProduct 0x1786 USB-N10 802.11n Network Adapter [Realtek RTL8188SU]
RealTek driver web site - -
http://www.realtek.com.tw/downloads/downloadsView.aspx?Langid=1&PNid=48&PFid=48&Level=5&Conn=4&DownTypeID=3&GetDown=false&Downloads=true#RTL8188SU
which claims support "Linux Kernel 2.6.18~2.6.38 and Kernel 3.0.2"
but requires I try compiling the driver from source
Andrew
^ permalink raw reply
* Re: [PATCH 05/19] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Patrick McHardy @ 2012-08-19 19:44 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netfilter-devel, netdev
In-Reply-To: <1345405069.3069.241.camel@localhost>
On Sun, 19 Aug 2012, Jesper Dangaard Brouer wrote:
> On Sat, 2012-08-18 at 14:26 +0200, Patrick McHardy wrote:
>>>
>>> Could you provide an iptables command/rule, that trigger this code path?
>>
>> The easiest way is a large ping with the NAT patches also applied,
>> in that case we also pass the first packet of a connection through
>> the stack reassembled.
>
> So, a fragmented IPv6 ICMPv6 packet, I assume?
Correct.
> Don't I need to load some of the helper modules, or just the
> nf_conntrack_ipv6 module, or perhaps only nf_defrag_ipv6 ?
Not with the entire patchset, just IPv6 conntrack is enough. Aith IPv6 NAT
the first packet of a connection must always be defragemented, independant
of an assigned helper.
>>>> @@ -199,9 +200,13 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
>>>> static unsigned int __ipv6_conntrack_in(struct net *net,
>>>> unsigned int hooknum,
>>>> struct sk_buff *skb,
>>>> + const struct net_device *in,
>>>> + const struct net_device *out,
>>>> int (*okfn)(struct sk_buff *))
>>>> {
>>>> struct sk_buff *reasm = skb->nfct_reasm;
>>>> + struct nf_conn *ct;
>>>> + enum ip_conntrack_info ctinfo;
>>>>
>>>> /* This packet is fragmented and has reassembled packet. */
>>>> if (reasm) {
>>>> @@ -213,6 +218,20 @@ static unsigned int __ipv6_conntrack_in(struct net *net,
>>>> if (ret != NF_ACCEPT)
>>>> return ret;
>>>> }
>>>> +
>>>> + /* Conntrack helpers need the entire reassembled packet in the
>>>> + * POST_ROUTING hook.
>>>> + */
>>>> + ct = nf_ct_get(reasm, &ctinfo);
>>>> + if (ct != NULL && test_bit(IPS_HELPER_BIT, &ct->status)) {
>>>> + nf_conntrack_get_reasm(skb);
>>>> + NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, reasm,
>>>> + (struct net_device *)in,
>>>> + (struct net_device *)out,
>>>> + okfn, NF_IP6_PRI_CONNTRACK + 1);
>>>
>>> Hook prio change to NF_IP6_PRI_CONNTRACK + 1
>>
>> I didn't get this part, you want to change to PRE_CONNTRACK + 1? What
>> about raw and SELinux?
>
> No - I don't want any changes.
>
> I was just pointing out *where* the changes occur in your patch. This is
> just a "service" to other email readers, so they can spot the changes
> faster, I were referring to.
Could you send me your patch so I get a better picture of what you're
doing exactly?
^ permalink raw reply
* Re: [PATCH 05/19] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Jesper Dangaard Brouer @ 2012-08-19 19:37 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netfilter-devel, netdev
In-Reply-To: <Pine.GSO.4.63.1208181423050.17600@stinky-local.trash.net>
On Sat, 2012-08-18 at 14:26 +0200, Patrick McHardy wrote:
> On Fri, 17 Aug 2012, Jesper Dangaard Brouer wrote:
>
> > On Thu, 2012-08-09 at 22:08 +0200, kaber@trash.net wrote:
> >> From: Patrick McHardy <kaber@trash.net>
> >>
> >> The IPv6 conntrack fragmentation currently has a couple of shortcomings.
> >> Fragmentes are collected in PREROUTING/OUTPUT, are defragmented, the
> >> defragmented packet is then passed to conntrack, the resulting conntrack
> >> information is attached to each original fragment and the fragments then
> >> continue their way through the stack.
> >>
> >> Helper invocation occurs in the POSTROUTING hook, at which point only
> >> the original fragments are available. The result of this is that
> >> fragmented packets are never passed to helpers.
> >>
> >> This patch improves the situation in the following way:
> >>
> >> - If a reassembled packet belongs to a connection that has a helper
> >> assigned, the reassembled packet is passed through the stack instead
> >> of the original fragments.
> >
> > I'm working on IPv6 fragment handling for IPVS, and are taking advantage
> > of the "replay" by nf_ct_frag6_output() at hook prio -399
> > (NF_IP6_PRI_CONNTRACK_DEFRAG + 1).
> > By making a hook at NF_INET_PRE_ROUTING at prio -99 (NF_IP6_PRI_NAT_DST
> > + 1).
> >
> > I can see that the code path can be changed (with this patch), if a
> > helper is assigned. Then the "replay" starts at prio -199
> > (NF_IP6_PRI_CONNTRACK + 1), I guess I'm safe as I run at -99.
> >
> > I have tested that your patchset works, with my ipvs patches, but would
> > like the trigger the changed code path, to make sure.
> >
> > Could you provide an iptables command/rule, that trigger this code path?
>
> The easiest way is a large ping with the NAT patches also applied,
> in that case we also pass the first packet of a connection through
> the stack reassembled.
So, a fragmented IPv6 ICMPv6 packet, I assume?
Don't I need to load some of the helper modules, or just the
nf_conntrack_ipv6 module, or perhaps only nf_defrag_ipv6 ?
> >> @@ -199,9 +200,13 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
> >> static unsigned int __ipv6_conntrack_in(struct net *net,
> >> unsigned int hooknum,
> >> struct sk_buff *skb,
> >> + const struct net_device *in,
> >> + const struct net_device *out,
> >> int (*okfn)(struct sk_buff *))
> >> {
> >> struct sk_buff *reasm = skb->nfct_reasm;
> >> + struct nf_conn *ct;
> >> + enum ip_conntrack_info ctinfo;
> >>
> >> /* This packet is fragmented and has reassembled packet. */
> >> if (reasm) {
> >> @@ -213,6 +218,20 @@ static unsigned int __ipv6_conntrack_in(struct net *net,
> >> if (ret != NF_ACCEPT)
> >> return ret;
> >> }
> >> +
> >> + /* Conntrack helpers need the entire reassembled packet in the
> >> + * POST_ROUTING hook.
> >> + */
> >> + ct = nf_ct_get(reasm, &ctinfo);
> >> + if (ct != NULL && test_bit(IPS_HELPER_BIT, &ct->status)) {
> >> + nf_conntrack_get_reasm(skb);
> >> + NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, reasm,
> >> + (struct net_device *)in,
> >> + (struct net_device *)out,
> >> + okfn, NF_IP6_PRI_CONNTRACK + 1);
> >
> > Hook prio change to NF_IP6_PRI_CONNTRACK + 1
>
> I didn't get this part, you want to change to PRE_CONNTRACK + 1? What
> about raw and SELinux?
No - I don't want any changes.
I was just pointing out *where* the changes occur in your patch. This is
just a "service" to other email readers, so they can spot the changes
faster, I were referring to.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: regression with poll(2)
From: Sage Weil @ 2012-08-19 18:49 UTC (permalink / raw)
To: mgorman, davem, netdev
Cc: linux-kernel, ceph-devel, neilb, a.p.zijlstra, michaelc, emunson,
eric.dumazet, sebastian, cl, akpm, torvalds
In-Reply-To: <alpine.DEB.2.00.1208151226250.15721@cobra.newdream.net>
I've bisected and identified this commit:
netvm: propagate page->pfmemalloc to skb
The skb->pfmemalloc flag gets set to true iff during the slab allocation
of data in __alloc_skb that the the PFMEMALLOC reserves were used. If the
packet is fragmented, it is possible that pages will be allocated from the
PFMEMALLOC reserve without propagating this information to the skb. This
patch propagates page->pfmemalloc from pages allocated for fragments to
the skb.
Signed-off-by: Mel Gorman <mgorman@suse.de>
Acked-by: David S. Miller <davem@davemloft.net>
Cc: Neil Brown <neilb@suse.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Eric B Munson <emunson@mgebm.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
I've retested several times and confirmed that this change leads to the
breakage, and also confirmed that reverting it on top of -rc1 also fixes
the problem.
I've also added some additional instrumentation to my code and confirmed
that the process is blocking on poll(2) while netstat is reporting
data available on the socket.
What can I do to help track this down?
Thanks!
sage
On Wed, 15 Aug 2012, Sage Weil wrote:
> I'm experiencing a stall with Ceph daemons communicating over TCP that
> occurs reliably with 3.6-rc1 (and linus/master) but not 3.5. The basic
> situation is:
>
> - the socket is two processes communicating over TCP on the same host, e.g.
>
> tcp 0 2164849 10.214.132.38:6801 10.214.132.38:51729 ESTABLISHED
>
> - one end writes a bunch of data in
> - the other end consumes data, but at some point stalls.
> - reads are nonblocking, e.g.
>
> int got = ::recv( sd, buf, len, MSG_DONTWAIT );
>
> and between those calls we wait with
>
> struct pollfd pfd;
> short evmask;
> pfd.fd = sd;
> pfd.events = POLLIN;
> #if defined(__linux__)
> pfd.events |= POLLRDHUP;
> #endif
>
> if (poll(&pfd, 1, msgr->timeout) <= 0)
> return -1;
>
> - in my case the timeout is ~15 minutes. at that point it errors out,
> and the daemons reconnect and continue for a while until hitting this
> again.
>
> - at the time of the stall, the reading process is blocked on that
> poll(2) call. There are a bunch of threads stuck on poll(2), some of them
> stuck and some not, but they all have stacks like
>
> [<ffffffff8118f6f9>] poll_schedule_timeout+0x49/0x70
> [<ffffffff81190baf>] do_sys_poll+0x35f/0x4c0
> [<ffffffff81190deb>] sys_poll+0x6b/0x100
> [<ffffffff8163d369>] system_call_fastpath+0x16/0x1b
>
> - you'll note that the netstat output shows data queued:
>
> tcp 0 1163264 10.214.132.36:6807 10.214.132.36:41738 ESTABLISHED
> tcp 0 1622016 10.214.132.36:41738 10.214.132.36:6807 ESTABLISHED
>
> etc.
>
> Is this a known regression? Or might I be misusing the API? What
> information would help track it down?
>
> Thanks!
> sage
>
>
>
^ permalink raw reply
* [PATCH] serial: add a new helper function
From: Huang Shijie @ 2012-08-19 18:27 UTC (permalink / raw)
To: gregkh; +Cc: alan, jirislaby, linux-kernel, linux-serial, netdev, Huang Shijie
In most of the time, the driver needs to check if the cts flow control
is enabled. But now, the driver checks the ASYNC_CTS_FLOW flag manually,
which is not a grace way. So add a new wraper function to make the code
tidy and clean.
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
drivers/char/pcmcia/synclink_cs.c | 2 +-
drivers/tty/amiserial.c | 2 +-
drivers/tty/cyclades.c | 2 +-
drivers/tty/isicom.c | 2 +-
drivers/tty/mxser.c | 2 +-
drivers/tty/serial/mxs-auart.c | 2 +-
drivers/tty/serial/serial_core.c | 4 ++--
drivers/tty/synclink.c | 2 +-
drivers/tty/synclink_gt.c | 2 +-
drivers/tty/synclinkmp.c | 2 +-
include/linux/tty.h | 7 +++++++
net/irda/ircomm/ircomm_tty.c | 4 ++--
net/irda/ircomm/ircomm_tty_attach.c | 2 +-
13 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 5db08c7..3f57d5de 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -1050,7 +1050,7 @@ static void cts_change(MGSLPC_INFO *info, struct tty_struct *tty)
wake_up_interruptible(&info->status_event_wait_q);
wake_up_interruptible(&info->event_wait_q);
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (info->serial_signals & SerialSignal_CTS) {
if (debug_level >= DEBUG_LEVEL_ISR)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 2b7535d..42d0a25 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -420,7 +420,7 @@ static void check_modem_status(struct serial_state *info)
tty_hangup(port->tty);
}
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (port->tty->hw_stopped) {
if (!(status & SER_CTS)) {
#if (defined(SERIAL_DEBUG_INTR) || defined(SERIAL_DEBUG_FLOW))
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index e3954da..0a6a0bc 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -727,7 +727,7 @@ static void cyy_chip_modem(struct cyclades_card *cinfo, int chip,
else
tty_hangup(tty);
}
- if ((mdm_change & CyCTS) && (info->port.flags & ASYNC_CTS_FLOW)) {
+ if ((mdm_change & CyCTS) && tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (mdm_status & CyCTS) {
/* cy_start isn't used
diff --git a/drivers/tty/isicom.c b/drivers/tty/isicom.c
index 99cf22e..d7492e1 100644
--- a/drivers/tty/isicom.c
+++ b/drivers/tty/isicom.c
@@ -600,7 +600,7 @@ static irqreturn_t isicom_interrupt(int irq, void *dev_id)
port->status &= ~ISI_DCD;
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (header & ISI_CTS) {
port->port.tty->hw_stopped = 0;
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index bb2da4c..cfda47d 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -830,7 +830,7 @@ static void mxser_check_modem_status(struct tty_struct *tty,
wake_up_interruptible(&port->port.open_wait);
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (status & UART_MSR_CTS) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 3a667ee..dafeef2 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -262,7 +262,7 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
ctrl &= ~AUART_CTRL2_RTSEN;
if (mctrl & TIOCM_RTS) {
- if (u->state->port.flags & ASYNC_CTS_FLOW)
+ if (tty_port_cts_enabled(&u->state->port))
ctrl |= AUART_CTRL2_RTSEN;
}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5b308c8..1920e5c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -176,7 +176,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
spin_lock_irq(&uport->lock);
if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
tty->hw_stopped = 1;
@@ -2493,7 +2493,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
uport->icount.cts++;
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (tty->hw_stopped) {
if (status) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index 666aa14..70e3a52 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -1359,7 +1359,7 @@ static void mgsl_isr_io_pin( struct mgsl_struct *info )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if (info->port.tty->hw_stopped) {
if (status & MISCSTATUS_CTS) {
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 45f6136..b38e954 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -2053,7 +2053,7 @@ static void cts_change(struct slgt_info *info, unsigned short status)
wake_up_interruptible(&info->event_wait_q);
info->pending_bh |= BH_STATUS;
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (info->port.tty) {
if (info->port.tty->hw_stopped) {
if (info->signals & SerialSignal_CTS) {
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index 53429c8..f17d9f3 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -2500,7 +2500,7 @@ static void isr_io_pin( SLMP_INFO *info, u16 status )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if ( info->port.tty ) {
if (info->port.tty->hw_stopped) {
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 69a787f..e0b4871 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -43,6 +43,7 @@
#include <linux/tty_driver.h>
#include <linux/tty_ldisc.h>
#include <linux/mutex.h>
+#include <linux/serial.h>
@@ -513,6 +514,12 @@ static inline struct tty_port *tty_port_get(struct tty_port *port)
return port;
}
+/* If the cts flow control is enabled, return true. */
+static inline bool tty_port_cts_enabled(struct tty_port *port)
+{
+ return port->flags & ASYNC_CTS_FLOW;
+}
+
extern struct tty_struct *tty_port_tty_get(struct tty_port *port);
extern void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty);
extern int tty_port_carrier_raised(struct tty_port *port);
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9668990..95a3a7a 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -1070,7 +1070,7 @@ void ircomm_tty_check_modem_status(struct ircomm_tty_cb *self)
goto put;
}
}
- if (tty && self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty && tty_port_cts_enabled(&self->port)) {
if (tty->hw_stopped) {
if (status & IRCOMM_CTS) {
IRDA_DEBUG(2,
@@ -1313,7 +1313,7 @@ static void ircomm_tty_line_info(struct ircomm_tty_cb *self, struct seq_file *m)
seq_puts(m, "Flags:");
sep = ' ';
- if (self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&self->port)) {
seq_printf(m, "%cASYNC_CTS_FLOW", sep);
sep = '|';
}
diff --git a/net/irda/ircomm/ircomm_tty_attach.c b/net/irda/ircomm/ircomm_tty_attach.c
index 3ab70e7..edab393 100644
--- a/net/irda/ircomm/ircomm_tty_attach.c
+++ b/net/irda/ircomm/ircomm_tty_attach.c
@@ -578,7 +578,7 @@ void ircomm_tty_link_established(struct ircomm_tty_cb *self)
* will have to wait for the peer device (DCE) to raise the CTS
* line.
*/
- if ((self->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&self->port) &&
((self->settings.dce & IRCOMM_CTS) == 0)) {
IRDA_DEBUG(0, "%s(), waiting for CTS ...\n", __func__ );
goto put;
--
1.7.4.4
^ permalink raw reply related
* Re: BUG: unable to handle kernel paging request at 00010016
From: Shaun Ruffell @ 2012-08-19 17:19 UTC (permalink / raw)
To: Artem Savkov; +Cc: Dave Haywood, Linux Kernel, netdev
In-Reply-To: <20120819082117.GA4634@thinkpad.lan>
[ Fixing netdev cc to use proper dadress... ]
On Sun, Aug 19, 2012 at 12:21:17PM +0400, Artem Savkov wrote:
> On Sat, Aug 18, 2012 at 11:25:43PM -0500, Shaun Ruffell wrote:
> > Adding linux-net to the CC list.
> >
> > On Fri, Aug 17, 2012 at 11:57:56PM +0100, Dave Haywood wrote:
> > > [1.] One line summary of the problem:
> > > BUG: unable to handle kernel paging request at 00010016
> > >
> > > System boots then crashes a 5-10 or so seconds after getting to the login prompt
> > > Booting without the network cable attached prevents the crash (no evidence beyond 10 minutes after boot)
> > >
> > > Diagnostics:
> > > Captured the boot and managed a login + dmesg before the crash
> > > Some of the log looks corrupted. Probably my crappy usb dongle serial flow control but left it in anyway
> >
> > [snip]
> >
> > Just a note that I see this as well. It happens reliably for me after trying to
> > login to the machine via ssh.
> >
> > Here is the back trace I collected on the serial port.
>
> There is a patch posted on netdev that fixes this for me:
> http://patchwork.ozlabs.org/patch/178525/
Thanks, I just applied that patch and confirmed that it does indeed
resolve the crash for me as well.
Cheers,
Shaun
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Julian Anastasov @ 2012-08-19 16:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
Hello,
On Sun, 19 Aug 2012, Eric Dumazet wrote:
> Hmm, this looks like sk_reset_timer() is called on a socket, and timer
> triggers _before_ the sock_hold()
>
> So the timer handler decrements sk_refcnt to 0 and calls sk_free()
>
> Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp:
> improve latencies of timer triggered events)
>
> I always found sk_reset_timer() a bit racy...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> if (!mod_timer(timer, expires))
> sock_hold(sk); // MIGHT BE TOO LATE
> }
>
> Following should be safer...
Above code is fine as long as caller holds reference.
Your change for tcp_release_cb looks correct.
Also, may be tcp_v4_mtu_reduced is missing a check
for TCP_CLOSE state? tcp_v6_mtu_reduced already has such
check.
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
This should not be needed:
> sock_hold(sk);
> if (mod_timer(timer, expires))
> sock_put(sk);
> }
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-19 16:17 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120819141641.GA9082@Krystal>
On 08/19/2012 04:16 PM, Mathieu Desnoyers wrote:
> * Mathieu Desnoyers (mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org) wrote:
>> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> [...]
>>> +/**
>>> + * hash_for_each_possible - iterate over all possible objects for a given key
>>> + * @name: hashtable to iterate
>>> + * @obj: the type * to use as a loop cursor for each bucket
>>> + * @bits: bit count of hashing function of the hashtable
>>> + * @node: the &struct list_head to use as a loop cursor for each bucket
>>> + * @member: the name of the hlist_node within the struct
>>> + * @key: the key of the objects to iterate over
>>> + */
>>> +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \
>>> + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member)
>>
>> Second point: "for_each_possible" does not express the iteration scope.
>> Citing WordNet: "possible adj 1: capable of happening or existing;" --
>> which has nothing to do with iteration on duplicate keys within a hash
>> table.
>>
>> I would recommend to rename "possible" to "duplicate", e.g.:
>>
>> hash_for_each_duplicate()
>>
>> which clearly says what is the scope of this iteration: duplicate keys.
>
> OK, about this part: I now see that you iterate over all objects within
> the same hash chain. I guess the description "iterate over all possible
> objects for a given key" is misleading: it's not all objects with a
> given key, but rather all objects hashing to the same bucket.
>
> I understand that you don't want to build knowledge of the key
> comparison function in the iterator (which makes sense for a simple hash
> table).
>
> By the way, the comment "@obj: the type * to use as a loop cursor for
> each bucket" is also misleading: it's a loop cursor for each entry,
> since you iterate on all nodes within single bucket. Same for "@node:
> the &struct list_head to use as a loop cursor for each bucket".
>
> So with these documentation changes applied, hash_for_each_possible
> starts to make more sense, because it refers to entries that can
> _possibly_ be a match (or not). Other options would be
> hash_chain_for_each() or hash_bucket_for_each().
I'd rather avoid starting to use chain/bucket since they're not used anywhere
else (I've tried keeping internal hashing/bucketing opaque to the user).
Otherwise makes sense, I'll improve the documentation as suggested. Thanks!
> Thanks,
>
> Mathieu
>
^ permalink raw reply
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-19 16:08 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120819131637.GA8272@Krystal>
On 08/19/2012 03:16 PM, Mathieu Desnoyers wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
>> This hashtable implementation is using hlist buckets to provide a simple
>> hashtable to prevent it from getting reimplemented all over the kernel.
>>
>> Signed-off-by: Sasha Levin <levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> include/linux/hashtable.h | 284 +++++++++++++++++++++++++++++++++++++++++++++
> [...]
>
> Hi Sasha,
>
> There are still a few API naming nits that I'd like to discuss:
>
>> +
>> +/**
>> + * hash_for_each_size - iterate over a hashtable
>> + * @name: hashtable to iterate
>> + * @bits: bit count of hashing function of the hashtable
>> + * @bkt: integer to use as bucket loop cursor
>> + * @node: the &struct list_head to use as a loop cursor for each bucket
>> + * @obj: the type * to use as a loop cursor for each bucket
>> + * @member: the name of the hlist_node within the struct
>> + */
>> +#define hash_for_each_size(name, bits, bkt, node, obj, member) \
>
> What is the meaning of "for each size" ?
>
> By looking at the implementation, I see that it takes an extra "bits"
> argument to specify the key width.
>
> But in the other patches of this patchset, I cannot find a single user
> of the "*_size" API. If you do not typically expect users to specify
> this parameter by hand (thanks to use of HASH_BITS(name) in for_each
> functions that do not take the bits parameter), I would recommend to
> only expose hash_for_each() and similar defines, but n I'd ot the *_size
> variants.
>
> So I recommend merging hash_for_each_size into hash_for_each (and
> doing similarly for other *_size variants). On the plus side, it will
> cut down the number of for_each macros from 12 down to 6, whiy och is more
> reasonable.
This is actually how the hashtable API was looking in the first place - without
the _size functions.
The story here is that when I've introduced the original API which used macro
magic to work out the size, one of the first comments was that there are several
dynamically allocated hashtables around the kernels, and all this macro magic
wouldn't work.
I've grepped around the code and indeed saw several places which k*alloc/vmalloc
their hashtable, so I agreed with that and happily went ahead to extend the API
to have _size functions.
When I started converting more kernel code to use this new API, I also converted
two modules which kmalloced the hashtable, but instead of using the _size API I
ended up removing the allocation completely because it was unnecessary and
wasteful. And this is why you don't see _size being used anywhere in any of the
patches.
Looking at the kernel code again, I see several places where removal of dynamic
allocation won't work (see 'new_tl_hash' in drivers/block/drbd/drbd_nl.c for
example).
So I'd rather leave it in at least until we finish converting, as I see several
places which will definitely need it.
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 15:55 UTC (permalink / raw)
To: Lin Ming; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <CAF1ivSZK8Y2XpjUXVpNg=m=bs7bGCYqF0ZQU2u_YpEUrmOrR-g@mail.gmail.com>
On Sun, 2012-08-19 at 23:05 +0800, Lin Ming wrote:
> On Sun, Aug 19, 2012 at 10:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
> >
> >> Will it still has problem if code goes here without sock_hold(sk)?
> >
> > Not sure of what you mean.
>
> See my comments in the function.
> Is that a potential problem?
>
No problem.
It always been like that. Thats the whole point having a refcount at the
first place.
The last sock_put(sk) should free the socket.
^ permalink raw reply
* Re: [PATCH] serial: add a new helper function
From: Alan Cox @ 2012-08-19 15:46 UTC (permalink / raw)
To: Greg KH; +Cc: Huang Shijie, alan, jirislaby, linux-kernel, linux-serial, netdev
In-Reply-To: <20120819064429.GA3252@kroah.com>
On Sat, 18 Aug 2012 23:44:29 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sun, Aug 19, 2012 at 02:27:12PM -0400, Huang Shijie wrote:
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -43,6 +43,7 @@
> > #include <linux/tty_driver.h>
> > #include <linux/tty_ldisc.h>
> > #include <linux/mutex.h>
> > +#include <linux/serial.h>
> >
> >
> >
> > @@ -513,6 +514,12 @@ static inline struct tty_port *tty_port_get(struct tty_port *port)
> > return port;
> > }
> >
> > +/* If the cts flow control is enabled, return true. */
> > +static inline bool tty_port_cts_enabled(struct tty_port *port)
> > +{
> > + return port->flags & ASYNC_CTS_FLOW;
> > +}
> > +
>
> The fact that you have to add serial.h to this file kind of implies that
> this function shouldn't be here, right?
>
> How about serial.h instead? Not all tty drivers are serial drivers :)
tty_port is tty generic so possibly if there is a generic helper the
flags and helper should likewise be this way.
As it stands at the moment ASYNC_CTS_FLOW is a convention a few drivers
use. So calling it tty_port_xxx is going to misleading.
Alan
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Lin Ming @ 2012-08-19 15:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345387532.5158.222.camel@edumazet-glaptop>
On Sun, Aug 19, 2012 at 10:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
>
>> Will it still has problem if code goes here without sock_hold(sk)?
>
> Not sure of what you mean.
See my comments in the function.
Is that a potential problem?
static void tcp_write_timer(unsigned long data)
{
struct sock *sk = (struct sock *)data;
bh_lock_sock(sk);
if (!sock_owned_by_user(sk)) {
tcp_write_timer_handler(sk);
//if we arrive here with sk_refcnt=1
//then below sock_put(sk) will also free the sock.
} else {
/* deleguate our work to tcp_release_cb() */
set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
}
bh_unlock_sock(sk);
sock_put(sk);
}
>
> At the time tcp_write_timer() runs, we own one reference on the socket.
> (this reference was taken in sk_reset_timer())
>
> On old kernels, if we found the socket locked by the user, we used to
> rearm the timer for a 50ms delay (and thus did sock_hold() again)
>
> Another way to avoid the bug would to make sure sk_reset_timer()
> increases refcount _before_ setting the timer, but its adding one atomic
> in fast path...
I think this is safer than your previous fix.
Thanks.
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8f67ced..d1745b7 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2114,8 +2114,9 @@ EXPORT_SYMBOL(sk_send_sigurg);
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> - if (!mod_timer(timer, expires))
> - sock_hold(sk);
> + sock_hold(sk);
> + if (mod_timer(timer, expires))
> + __sock_put(sk);
> }
> EXPORT_SYMBOL(sk_reset_timer);
>
>
>
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 14:45 UTC (permalink / raw)
To: Lin Ming; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <CAF1ivSaU_ur9BkKG3eAsNw7d+ND5xgYbPKutemVqNec3X9m4SA@mail.gmail.com>
On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
> Will it still has problem if code goes here without sock_hold(sk)?
Not sure of what you mean.
At the time tcp_write_timer() runs, we own one reference on the socket.
(this reference was taken in sk_reset_timer())
On old kernels, if we found the socket locked by the user, we used to
rearm the timer for a 50ms delay (and thus did sock_hold() again)
Another way to avoid the bug would to make sure sk_reset_timer()
increases refcount _before_ setting the timer, but its adding one atomic
in fast path...
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..d1745b7 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2114,8 +2114,9 @@ EXPORT_SYMBOL(sk_send_sigurg);
void sk_reset_timer(struct sock *sk, struct timer_list* timer,
unsigned long expires)
{
- if (!mod_timer(timer, expires))
- sock_hold(sk);
+ sock_hold(sk);
+ if (mod_timer(timer, expires))
+ __sock_put(sk);
}
EXPORT_SYMBOL(sk_reset_timer);
^ permalink raw reply related
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-08-19 14:16 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120819131637.GA8272@Krystal>
* Mathieu Desnoyers (mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org) wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
[...]
> > +/**
> > + * hash_for_each_possible - iterate over all possible objects for a given key
> > + * @name: hashtable to iterate
> > + * @obj: the type * to use as a loop cursor for each bucket
> > + * @bits: bit count of hashing function of the hashtable
> > + * @node: the &struct list_head to use as a loop cursor for each bucket
> > + * @member: the name of the hlist_node within the struct
> > + * @key: the key of the objects to iterate over
> > + */
> > +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \
> > + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member)
>
> Second point: "for_each_possible" does not express the iteration scope.
> Citing WordNet: "possible adj 1: capable of happening or existing;" --
> which has nothing to do with iteration on duplicate keys within a hash
> table.
>
> I would recommend to rename "possible" to "duplicate", e.g.:
>
> hash_for_each_duplicate()
>
> which clearly says what is the scope of this iteration: duplicate keys.
OK, about this part: I now see that you iterate over all objects within
the same hash chain. I guess the description "iterate over all possible
objects for a given key" is misleading: it's not all objects with a
given key, but rather all objects hashing to the same bucket.
I understand that you don't want to build knowledge of the key
comparison function in the iterator (which makes sense for a simple hash
table).
By the way, the comment "@obj: the type * to use as a loop cursor for
each bucket" is also misleading: it's a loop cursor for each entry,
since you iterate on all nodes within single bucket. Same for "@node:
the &struct list_head to use as a loop cursor for each bucket".
So with these documentation changes applied, hash_for_each_possible
starts to make more sense, because it refers to entries that can
_possibly_ be a match (or not). Other options would be
hash_chain_for_each() or hash_bucket_for_each().
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Lin Ming @ 2012-08-19 14:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
On Sun, Aug 19, 2012 at 8:51 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
<snip...>
> Hi Fengguang, thanks for this report.
>
> Hmm, this looks like sk_reset_timer() is called on a socket, and timer
> triggers _before_ the sock_hold()
>
> So the timer handler decrements sk_refcnt to 0 and calls sk_free()
>
> Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp:
> improve latencies of timer triggered events)
>
> I always found sk_reset_timer() a bit racy...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> if (!mod_timer(timer, expires))
> sock_hold(sk); // MIGHT BE TOO LATE
> }
>
> Following should be safer...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> sock_hold(sk);
> if (mod_timer(timer, expires))
> sock_put(sk);
> }
>
> Could you test following patch ?
>
> By the way, there is a typo in tcp_delack_timer()
> (should use TCP_DELACK_TIMER_DEFERRED instead of
> TCP_WRITE_TIMER_DEFERRED)
>
>
> Thanks !
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 7678237..6278a11 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -417,10 +417,12 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
>
> if (code == ICMP_FRAG_NEEDED) { /* PMTU discovery (RFC1191) */
> tp->mtu_info = info;
> - if (!sock_owned_by_user(sk))
> + if (!sock_owned_by_user(sk)) {
> tcp_v4_mtu_reduced(sk);
> - else
> - set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags);
> + } else {
> + if (!test_and_set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags))
> + sock_hold(sk);
> + }
> goto out;
> }
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 20dfd89..d046326 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -910,14 +910,18 @@ void tcp_release_cb(struct sock *sk)
> if (flags & (1UL << TCP_TSQ_DEFERRED))
> tcp_tsq_handler(sk);
>
> - if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED))
> + if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) {
> tcp_write_timer_handler(sk);
> -
> - if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED))
> + __sock_put(sk);
> + }
> + if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) {
> tcp_delack_timer_handler(sk);
> -
> - if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED))
> + __sock_put(sk);
> + }
> + if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) {
> sk->sk_prot->mtu_reduced(sk);
> + __sock_put(sk);
> + }
> }
> EXPORT_SYMBOL(tcp_release_cb);
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 6df36ad..b774a03 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -252,7 +252,8 @@ static void tcp_delack_timer(unsigned long data)
> inet_csk(sk)->icsk_ack.blocked = 1;
> NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOCKED);
> /* deleguate our work to tcp_release_cb() */
> - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
> + if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
> + sock_hold(sk);
> }
> bh_unlock_sock(sk);
> sock_put(sk);
> @@ -481,7 +482,8 @@ static void tcp_write_timer(unsigned long data)
> tcp_write_timer_handler(sk);
Will it still has problem if code goes here without sock_hold(sk)?
> } else {
> /* deleguate our work to tcp_release_cb() */
> - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
> + if (!test_and_set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
> + sock_hold(sk);
> }
> bh_unlock_sock(sk);
> sock_put(sk);
>
^ permalink raw reply
* Re: [RFC] Interface for TCP Metrics
From: Eric Dumazet @ 2012-08-19 13:50 UTC (permalink / raw)
To: Julian Anastasov; +Cc: netdev
In-Reply-To: <alpine.LFD.2.00.1208191402560.1893@ja.ssi.bg>
On Sun, 2012-08-19 at 14:42 +0300, Julian Anastasov wrote:
> Hello,
>
> Once DaveM mentioned that TCP Metrics need their
> own interface. I'm planning to implement such interface
> but lets first decide how it should look. After little
> research, here is my plan:
>
> - will use genl with TCP_METRICS_GENL_NAME "tcp_metrics",
> TCP_METRICS_GENL_VERSION 0x01
>
> - provide dumpit method and one cmd to read metrics by exact addr,
> will use TCP_METRICS_CMD_{GET,...} and TCP_METRICS_ATTR_xxx in
> new file include/linux/tcp_metrics.h
>
> - Is command to delete cached entry needed? Delete will need
> new rcu_head. Useful to flush the cache or to delete entries
> with filter.
>
> - without support for delete cmd, may be we can add command to
> reset entry with default values from dst?
>
> - Where to put the new netlink code?
> tcp_metrics_netlink.c
> tcp_metrics_nl.c
> or just in current tcp_metrics.c ?
>
> - will provide support for ip tool:
>
> ip tcpm[etrics] <cmd> ...
> ip -6 tcpm[etrics] get <addr> => TCP_METRICS_CMD_GET for 1 entry
> ip tcpm[etrics] list => dumpit
>
> - any wishes how to look the output? How to select specific
> values/metrics in output? For example, list only specified
> named values: ip tcpm list [-o] rtt
>
> - command to modify specific metric for addr, by name? Only
> for tcpm_vals? If not locked?
>
> Do we need modify/delete/reset support or just read
> support is enough? Comments?
>
> Regards
One way to delete one or all entries would be good, for being able to
reproduce some tests from a known state.
^ permalink raw reply
* [PATCH net-next] ipv6: gre: fix ip6gre_err()
From: Eric Dumazet @ 2012-08-19 13:47 UTC (permalink / raw)
To: Dmitry Kozlov; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
ip6gre_err() miscomputes grehlen (sizeof(ipv6h) is 4 or 8,
not 40 as expected), and should take into account 'offset' parameter.
Also uses pskb_may_pull() to cope with some fragged skbs
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Dmitry Kozlov <xeb@mail.ru>
---
net/ipv6/ip6_gre.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index a84ad5d..424d11a 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -415,8 +415,8 @@ static void ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
u8 type, u8 code, int offset, __be32 info)
{
const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)skb->data;
- __be16 *p = (__be16 *)(ipv6h + 1);
- int grehlen = sizeof(ipv6h) + 4;
+ __be16 *p = (__be16 *)(skb->data + offset);
+ int grehlen = offset + 4;
struct ip6_tnl *t;
__be16 flags;
@@ -432,8 +432,10 @@ static void ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
}
/* If only 8 bytes returned, keyed message will be dropped here */
- if (skb_headlen(skb) < grehlen)
+ if (!pskb_may_pull(skb, grehlen))
return;
+ ipv6h = (const struct ipv6hdr *)skb->data;
+ p = (__be16 *)(skb->data + offset);
rcu_read_lock();
^ permalink raw reply related
* Re: [PATCH net v2] net: tcp: move sk_rx_dst_set call after tcp_create_openreq_child()
From: Eric Dumazet @ 2012-08-19 13:41 UTC (permalink / raw)
To: Neal Cardwell; +Cc: David Miller, netdev, Eric Dumazet
In-Reply-To: <1345383038-8330-1-git-send-email-ncardwell@google.com>
On Sun, 2012-08-19 at 09:30 -0400, Neal Cardwell wrote:
> This commit removes the sk_rx_dst_set calls from
> tcp_create_openreq_child(), because at that point the icsk_af_ops
> field of ipv6_mapped TCP sockets has not been set to its proper final
> value.
>
> Instead, to make sure we get the right sk_rx_dst_set variant
> appropriate for the address family of the new connection, we have
> tcp_v{4,6}_syn_recv_sock() directly call the appropriate function
> shortly after the call to tcp_create_openreq_child() returns.
>
> This also moves inet6_sk_rx_dst_set() to avoid a forward declaration
> with the new approach.
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Reported-by: Artem Savkov <artem.savkov@gmail.com>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
> net/ipv4/tcp_ipv4.c | 1 +
> net/ipv4/tcp_minisocks.c | 2 --
> net/ipv6/tcp_ipv6.c | 25 +++++++++++++------------
> 3 files changed, 14 insertions(+), 14 deletions(-)
Thanks Neal !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [PATCH net v2] net: tcp: move sk_rx_dst_set call after tcp_create_openreq_child()
From: Neal Cardwell @ 2012-08-19 13:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Eric Dumazet, Neal Cardwell
This commit removes the sk_rx_dst_set calls from
tcp_create_openreq_child(), because at that point the icsk_af_ops
field of ipv6_mapped TCP sockets has not been set to its proper final
value.
Instead, to make sure we get the right sk_rx_dst_set variant
appropriate for the address family of the new connection, we have
tcp_v{4,6}_syn_recv_sock() directly call the appropriate function
shortly after the call to tcp_create_openreq_child() returns.
This also moves inet6_sk_rx_dst_set() to avoid a forward declaration
with the new approach.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reported-by: Artem Savkov <artem.savkov@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_ipv4.c | 1 +
net/ipv4/tcp_minisocks.c | 2 --
net/ipv6/tcp_ipv6.c | 25 +++++++++++++------------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7678237..5bf2040 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1462,6 +1462,7 @@ struct sock *tcp_v4_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
goto exit_nonewsk;
newsk->sk_gso_type = SKB_GSO_TCPV4;
+ inet_sk_rx_dst_set(newsk, skb);
newtp = tcp_sk(newsk);
newinet = inet_sk(newsk);
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index d9c9dce..6ff7f10 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -387,8 +387,6 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
struct tcp_sock *oldtp = tcp_sk(sk);
struct tcp_cookie_values *oldcvp = oldtp->cookie_values;
- newicsk->icsk_af_ops->sk_rx_dst_set(newsk, skb);
-
/* TCP Cookie Transactions require space for the cookie pair,
* as it differs for each connection. There is no need to
* copy any s_data_payload stored at the original socket.
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index bb9ce2b..a3e60cc 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -94,6 +94,18 @@ static struct tcp_md5sig_key *tcp_v6_md5_do_lookup(struct sock *sk,
}
#endif
+static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
+{
+ struct dst_entry *dst = skb_dst(skb);
+ const struct rt6_info *rt = (const struct rt6_info *)dst;
+
+ dst_hold(dst);
+ sk->sk_rx_dst = dst;
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
+ if (rt->rt6i_node)
+ inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
+}
+
static void tcp_v6_hash(struct sock *sk)
{
if (sk->sk_state != TCP_CLOSE) {
@@ -1270,6 +1282,7 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
newsk->sk_gso_type = SKB_GSO_TCPV6;
__ip6_dst_store(newsk, dst, NULL, NULL);
+ inet6_sk_rx_dst_set(newsk, skb);
newtcp6sk = (struct tcp6_sock *)newsk;
inet_sk(newsk)->pinet6 = &newtcp6sk->inet6;
@@ -1729,18 +1742,6 @@ static struct timewait_sock_ops tcp6_timewait_sock_ops = {
.twsk_destructor= tcp_twsk_destructor,
};
-static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
-{
- struct dst_entry *dst = skb_dst(skb);
- const struct rt6_info *rt = (const struct rt6_info *)dst;
-
- dst_hold(dst);
- sk->sk_rx_dst = dst;
- inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
- if (rt->rt6i_node)
- inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
-}
-
static const struct inet_connection_sock_af_ops ipv6_specific = {
.queue_xmit = inet6_csk_xmit,
.send_check = tcp_v6_send_check,
--
1.7.7.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox