* [PATCH 1/3] net: cpsw: Add a minimal cpsw-common module for shared code
From: Tony Lindgren @ 2015-01-28 19:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-omap, Brian Hutchinson, Felipe Balbi
In-Reply-To: <1422473586-18100-1-git-send-email-tony@atomide.com>
Looks like davinci_emac and cpsw can share some code although the
device registers have a different layout.
At least the code for getting the MAC address using syscon can
be shared by passing the register offset. Let's start with that
and set up a minimal shared cpsw-shared.c.
Cc: Brian Hutchinson <b.hutchman@gmail.com>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/net/ethernet/ti/Makefile | 3 ++
drivers/net/ethernet/ti/cpsw-common.c | 53 +++++++++++++++++++++++++++++++++++
drivers/net/ethernet/ti/cpsw.c | 35 ++---------------------
drivers/net/ethernet/ti/cpsw.h | 2 ++
4 files changed, 60 insertions(+), 33 deletions(-)
create mode 100644 drivers/net/ethernet/ti/cpsw-common.c
diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
index 465d03d..7fa47f6 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -2,6 +2,9 @@
# Makefile for the TI network device drivers.
#
+obj-$(CONFIG_TI_CPSW) += cpsw-common.o
+obj-$(CONFIG_TI_DAVINCI_EMAC) += cpsw-common.o
+
obj-$(CONFIG_TLAN) += tlan.o
obj-$(CONFIG_CPMAC) += cpmac.o
obj-$(CONFIG_TI_DAVINCI_EMAC) += davinci_emac.o
diff --git a/drivers/net/ethernet/ti/cpsw-common.c b/drivers/net/ethernet/ti/cpsw-common.c
new file mode 100644
index 0000000..763ada1
--- /dev/null
+++ b/drivers/net/ethernet/ti/cpsw-common.c
@@ -0,0 +1,53 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#define AM33XX_CTRL_MAC_LO_REG(offset, id) ((offset) + 0x8 * (id))
+#define AM33XX_CTRL_MAC_HI_REG(offset, id) ((offset) + 0x8 * (id) + 0x4)
+
+int cpsw_am33xx_cm_get_macid(struct device *dev, u16 offset, int slave,
+ u8 *mac_addr)
+{
+ u32 macid_lo;
+ u32 macid_hi;
+ struct regmap *syscon;
+
+ syscon = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
+ if (IS_ERR(syscon)) {
+ if (PTR_ERR(syscon) == -ENODEV)
+ return 0;
+ return PTR_ERR(syscon);
+ }
+
+ regmap_read(syscon, AM33XX_CTRL_MAC_LO_REG(offset, slave),
+ &macid_lo);
+ regmap_read(syscon, AM33XX_CTRL_MAC_HI_REG(offset, slave),
+ &macid_hi);
+
+ mac_addr[5] = (macid_lo >> 8) & 0xff;
+ mac_addr[4] = macid_lo & 0xff;
+ mac_addr[3] = (macid_hi >> 24) & 0xff;
+ mac_addr[2] = (macid_hi >> 16) & 0xff;
+ mac_addr[1] = (macid_hi >> 8) & 0xff;
+ mac_addr[0] = macid_hi & 0xff;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(cpsw_am33xx_cm_get_macid);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 2b9d404..7d8dd0d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -33,8 +33,6 @@
#include <linux/of_net.h>
#include <linux/of_device.h>
#include <linux/if_vlan.h>
-#include <linux/mfd/syscon.h>
-#include <linux/regmap.h>
#include <linux/pinctrl/consumer.h>
@@ -1936,36 +1934,6 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
slave->port_vlan = data->dual_emac_res_vlan;
}
-#define AM33XX_CTRL_MAC_LO_REG(id) (0x630 + 0x8 * id)
-#define AM33XX_CTRL_MAC_HI_REG(id) (0x630 + 0x8 * id + 0x4)
-
-static int cpsw_am33xx_cm_get_macid(struct device *dev, int slave,
- u8 *mac_addr)
-{
- u32 macid_lo;
- u32 macid_hi;
- struct regmap *syscon;
-
- syscon = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
- if (IS_ERR(syscon)) {
- if (PTR_ERR(syscon) == -ENODEV)
- return 0;
- return PTR_ERR(syscon);
- }
-
- regmap_read(syscon, AM33XX_CTRL_MAC_LO_REG(slave), &macid_lo);
- regmap_read(syscon, AM33XX_CTRL_MAC_HI_REG(slave), &macid_hi);
-
- mac_addr[5] = (macid_lo >> 8) & 0xff;
- mac_addr[4] = macid_lo & 0xff;
- mac_addr[3] = (macid_hi >> 24) & 0xff;
- mac_addr[2] = (macid_hi >> 16) & 0xff;
- mac_addr[1] = (macid_hi >> 8) & 0xff;
- mac_addr[0] = macid_hi & 0xff;
-
- return 0;
-}
-
static int cpsw_probe_dt(struct cpsw_platform_data *data,
struct platform_device *pdev)
{
@@ -2090,7 +2058,8 @@ no_phy_slave:
memcpy(slave_data->mac_addr, mac_addr, ETH_ALEN);
} else {
if (of_machine_is_compatible("ti,am33xx")) {
- ret = cpsw_am33xx_cm_get_macid(&pdev->dev, i,
+ ret = cpsw_am33xx_cm_get_macid(&pdev->dev,
+ 0x630, i,
slave_data->mac_addr);
if (ret)
return ret;
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index 1b71067..ca90efa 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -41,5 +41,7 @@ struct cpsw_platform_data {
};
void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave);
+int cpsw_am33xx_cm_get_macid(struct device *dev, u16 offset, int slave,
+ u8 *mac_addr);
#endif /* __CPSW_H__ */
--
2.1.4
^ permalink raw reply related
* [PATCH 0/3] Changes to cpsw and davinci_emac for getting MAC address
From: Tony Lindgren @ 2015-01-28 19:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-omap
Hi all,
Here are a few patches to add common code for cpsw and davinci_emac for
getting the MAC address. Looks like we can also now add code to get the
MAC address on 3517 but in a slightly different way.
Regards,
Tony
Tony Lindgren (3):
net: cpsw: Add a minimal cpsw-common module for shared code
net: davinci_emac: Get device dm816x MAC address using the cpsw code
net: davinci_emac: Get device MAC on 3517
arch/arm/boot/dts/am3517.dtsi | 1 +
drivers/net/ethernet/ti/Makefile | 3 ++
drivers/net/ethernet/ti/cpsw-common.c | 53 ++++++++++++++++++++++++++++++++
drivers/net/ethernet/ti/cpsw.c | 35 ++-------------------
drivers/net/ethernet/ti/cpsw.h | 2 ++
drivers/net/ethernet/ti/davinci_emac.c | 56 +++++++++++++++++++++++++++++++++-
6 files changed, 116 insertions(+), 34 deletions(-)
create mode 100644 drivers/net/ethernet/ti/cpsw-common.c
--
2.1.4
^ permalink raw reply
* Re: [PATCH 1/2] rhashtable: Introduce rhashtable_walk_*
From: Patrick McHardy @ 2015-01-28 19:07 UTC (permalink / raw)
To: Herbert Xu
Cc: Thomas Graf, David Miller, David.Laight, ying.xue, paulmck,
netdev, netfilter-devel
In-Reply-To: <20150127203657.GA9061@gondor.apana.org.au>
On 28.01, Herbert Xu wrote:
> On Tue, Jan 27, 2015 at 01:09:50PM +0000, Patrick McHardy wrote:
> >
> > Actually I have a patchset queued that adds runtime additions and
> > removals, both active and timeout based. So netfilter won't have
> > pure synchronous behaviour anymore.
>
> Can you show us the patchset?
Sure, will send it over tomorrow.
^ permalink raw reply
* [patch net-next] hisilicon: add some missing curly braces
From: Dan Carpenter @ 2015-01-28 18:58 UTC (permalink / raw)
To: dingtianhong
Cc: Zhangfei Gao, David S. Miller, Arnd Bergmann, netdev,
kernel-janitors
The if block was supposed to have curly braces. In the current code we
complain about dropped rx packets when we shouldn't.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 525214e..c02b81b 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -567,10 +567,11 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
if (unlikely(ists & DEF_INT_ERR)) {
- if (ists & (RCV_NOBUF | RCV_DROP))
+ if (ists & (RCV_NOBUF | RCV_DROP)) {
stats->rx_errors++;
stats->rx_dropped++;
netdev_err(ndev, "rx drop\n");
+ }
if (ists & TX_DROP) {
stats->tx_dropped++;
netdev_err(ndev, "tx drop\n");
^ permalink raw reply related
* [tip:irq/core PATCH] genirq: fix null pointer reference in irq_set_affinity_hint()
From: Jesse Brandeburg @ 2015-01-28 18:57 UTC (permalink / raw)
To: tglx; +Cc: mingo, netdev, yinghai, linux-kernel, hpa
The recent set_affinity commit by me introduced some null
pointer dereferences on driver unload, because some drivers
call this function with a NULL argument. This fixes the issue
by just checking for null before setting the affinity mask.
Fixes: e2e64a932556 ("genirq: Set initial affinity in irq_set_affinity_hint()")
Reported-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: netdev@vger.kernel.org
---
kernel/irq/manage.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f038e58..196a06f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -244,7 +244,8 @@ int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m)
desc->affinity_hint = m;
irq_put_desc_unlock(desc, flags);
/* set the initial affinity to prevent every interrupt being on CPU0 */
- __irq_set_affinity(irq, m, false);
+ if (m)
+ __irq_set_affinity(irq, m, false);
return 0;
}
EXPORT_SYMBOL_GPL(irq_set_affinity_hint);
^ permalink raw reply related
* Re: [PATCH net-next v8 0/4] net: Add Keystone NetCP ethernet driver support
From: Murali Karicheri @ 2015-01-28 18:20 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David Miller, devicetree, linux-kernel, netdev
In-Reply-To: <54C91FDD.1000009@ti.com>
On 01/28/2015 12:43 PM, Murali Karicheri wrote:
> On 01/28/2015 11:49 AM, Murali Karicheri wrote:
>> On 01/27/2015 05:28 PM, Arnd Bergmann wrote:
>>> On Tuesday 20 January 2015 10:53:36 Murali Karicheri wrote:
>>>> On 01/19/2015 03:11 PM, David Miller wrote:
>>>>> From: Murali Karicheri<m-karicheri2@ti.com>
>>>>> Date: Thu, 15 Jan 2015 19:10:03 -0500
>>>>>
>>>>>> The Network Coprocessor (NetCP) is a hardware accelerator that
>>>>>> processes
>>>>>> Ethernet packets. NetCP has a gigabit Ethernet (GbE) subsystem with
>>>>>> a ethernet
>>>>>> switch sub-module to send and receive packets. NetCP also includes
>>>>>> a packet
>>>>>> accelerator (PA) module to perform packet classification operations
>>>>>> such as
>>>>>> header matching, and packet modification operations such as checksum
>>>>>> generation. NetCP can also optionally include a Security
>>>>>> Accelerator(SA)
>>>>>> capable of performing IPSec operations on ingress/egress packets.
>>>>>>
>>>>>> Keystone SoC's also have a 10 Gigabit Ethernet Subsystem (XGbE) which
>>>>>> includes a 3-port Ethernet switch sub-module capable of 10Gb/s and
>>>>>> 1Gb/s rates per Ethernet port.
>>>>>>
>>>>>> Both GBE and XGBE network processors supported using common
>>>>>> driver. It
>>>>>> is also designed to handle future variants of NetCP.
>>>>>
>>>>> Series applied to net-next, thanks.
>>>> David,
>>>>
>>>> Thanks a lot for applying this series. This helps us move forward to
>>>> work on the next set of patches.
>>>
>>> Hi Murali,
>>>
>>> Building an ARM 'allmodconfig' kernel now runs into two separate
>>> problems
>>> from your driver:
>>>
>>> - you have two module_init() instances in one module, which conflict.
>>>
>>> - you have two files that are linked into more than one module, so
>>> building
>>> both TI_CPSW and TI_KEYSTONE_NETCP in the same kernel fails.
>>>
>>> The answer to both of these is probably to have separate loadable
>>> modules,
>>> but you might be able to come up with a different solution.
>> Arnd,
>>
>> Thanks for letting us know. We will look into this.
>>
>> How do I reproduce this? Is there a defconfig used for allmodconfig? I
>> am unable to find one. Any details to reproduce this will be useful.
>>
> Ok I think I found it.
>
> I did this with next-next branch and it seems to work. I will make
> kernel build to reproduce this.
>
> make ARCH=arm allmodconfig
> make uImage;
>
> I am building it now.
Arnd,
I see allmodconfig configure NetCP driver as module. My uImage build
from net-next branch went through fine. I am building modules right now
and should show error as you have pointed out. Let me know if you any
issues on how I am working to reproduce the issue (wrong branch, wrong
/incomplete commands etc. I have my CROSS_COMPILE and ARCH set in my
env). Want to reproduce this so as to make sure my fix is addressing
this. Hope I am on the right track.
Thanks
Murali
>
> Murali
>> Thanks.
>>
>> Murali
>>>
>>> Arnd
>>
>>
>
>
--
Murali Karicheri
Linux Kernel, Texas Instruments
^ permalink raw reply
* Re: [PATCH net-next v8 0/4] net: Add Keystone NetCP ethernet driver support
From: Murali Karicheri @ 2015-01-28 17:43 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David Miller, devicetree, linux-kernel, netdev
In-Reply-To: <54C91328.6030208@ti.com>
On 01/28/2015 11:49 AM, Murali Karicheri wrote:
> On 01/27/2015 05:28 PM, Arnd Bergmann wrote:
>> On Tuesday 20 January 2015 10:53:36 Murali Karicheri wrote:
>>> On 01/19/2015 03:11 PM, David Miller wrote:
>>>> From: Murali Karicheri<m-karicheri2@ti.com>
>>>> Date: Thu, 15 Jan 2015 19:10:03 -0500
>>>>
>>>>> The Network Coprocessor (NetCP) is a hardware accelerator that
>>>>> processes
>>>>> Ethernet packets. NetCP has a gigabit Ethernet (GbE) subsystem with
>>>>> a ethernet
>>>>> switch sub-module to send and receive packets. NetCP also includes
>>>>> a packet
>>>>> accelerator (PA) module to perform packet classification operations
>>>>> such as
>>>>> header matching, and packet modification operations such as checksum
>>>>> generation. NetCP can also optionally include a Security
>>>>> Accelerator(SA)
>>>>> capable of performing IPSec operations on ingress/egress packets.
>>>>>
>>>>> Keystone SoC's also have a 10 Gigabit Ethernet Subsystem (XGbE) which
>>>>> includes a 3-port Ethernet switch sub-module capable of 10Gb/s and
>>>>> 1Gb/s rates per Ethernet port.
>>>>>
>>>>> Both GBE and XGBE network processors supported using common driver. It
>>>>> is also designed to handle future variants of NetCP.
>>>>
>>>> Series applied to net-next, thanks.
>>> David,
>>>
>>> Thanks a lot for applying this series. This helps us move forward to
>>> work on the next set of patches.
>>
>> Hi Murali,
>>
>> Building an ARM 'allmodconfig' kernel now runs into two separate problems
>> from your driver:
>>
>> - you have two module_init() instances in one module, which conflict.
>>
>> - you have two files that are linked into more than one module, so
>> building
>> both TI_CPSW and TI_KEYSTONE_NETCP in the same kernel fails.
>>
>> The answer to both of these is probably to have separate loadable
>> modules,
>> but you might be able to come up with a different solution.
> Arnd,
>
> Thanks for letting us know. We will look into this.
>
> How do I reproduce this? Is there a defconfig used for allmodconfig? I
> am unable to find one. Any details to reproduce this will be useful.
>
Ok I think I found it.
I did this with next-next branch and it seems to work. I will make
kernel build to reproduce this.
make ARCH=arm allmodconfig
make uImage;
I am building it now.
Murali
> Thanks.
>
> Murali
>>
>> Arnd
>
>
--
Murali Karicheri
Linux Kernel, Texas Instruments
^ permalink raw reply
* Re: make allyesconfig i386 build failure with next-20150122 (caused by fb_agm1264k-fl driver)
From: Greg Kroah-Hartman @ 2015-01-28 17:36 UTC (permalink / raw)
To: Guenter Roeck
Cc: devel, Stephen Rothwell, isdn, netdev, linux-kernel, linux-next,
Jim Davis
In-Reply-To: <20150126215959.GA9853@roeck-us.net>
On Mon, Jan 26, 2015 at 01:59:59PM -0800, Guenter Roeck wrote:
> On Thu, Jan 22, 2015 at 12:10:33PM -0700, Jim Davis wrote:
> > make ARCH=i386 allyesconfig fails with
> >
> > drivers/staging/built-in.o: In function `reset':
> > (.text+0x2ae89d): multiple definition of `reset'
> > drivers/isdn/built-in.o:(.text+0x185dc2): first defined here
> > make[1]: *** [drivers/built-in.o] Error 1
>
> Culprit:
>
> commit b2ebd4be6fa1d2329b63531b044f9e25474981cb
> Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Date: Wed Dec 31 10:11:10 2014 +0100
>
> staging: fbtft: add fb_agm1264k-fl driver
>
> A global function named 'reset' isn't really a good idea.
>
> Not that the global function with the same name in the isdn code
> is better ;-).
It's been fixed in my tree for a while now :)
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Hannes Frederic Sowa @ 2015-01-28 17:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Vladislav Yasevich, virtualization, edumazet,
Ben Hutchings
In-Reply-To: <20150128164853.GA2963@redhat.com>
On Mi, 2015-01-28 at 18:48 +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 28, 2015 at 05:15:49PM +0100, Hannes Frederic Sowa wrote:
> > Hi,
> >
> > On Mi, 2015-01-28 at 18:00 +0200, Michael S. Tsirkin wrote:
> > > On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote:
> > > > Hi,
> > > >
> > > > On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> > > > > On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
> > > > > > Hello,
> > > > > >
> > > > > > On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
> > > > > > > On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
> > > > > > > > On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
> > > > > > > > > On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
> > > > > > > > > > On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
> > > > > > > > > >> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
> > > > > > > > > >>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
> > > > > > > > > >>>> If the IPv6 fragment id has not been set and we perform
> > > > > > > > > >>>> fragmentation due to UFO, select a new fragment id.
> > > > > > > > > >>>> When we store the fragment id into skb_shinfo, set the bit
> > > > > > > > > >>>> in the skb so we can re-use the selected id.
> > > > > > > > > >>>> This preserves the behavior of UFO packets generated on the
> > > > > > > > > >>>> host and solves the issue of id generation for packet sockets
> > > > > > > > > >>>> and tap/macvtap devices.
> > > > > > > > > >>>>
> > > > > > > > > >>>> This patch moves ipv6_select_ident() back in to the header file.
> > > > > > > > > >>>> It also provides the helper function that sets skb_shinfo() frag
> > > > > > > > > >>>> id and sets the bit.
> > > > > > > > > >>>>
> > > > > > > > > >>>> It also makes sure that we select the fragment id when doing
> > > > > > > > > >>>> just gso validation, since it's possible for the packet to
> > > > > > > > > >>>> come from an untrusted source (VM) and be forwarded through
> > > > > > > > > >>>> a UFO enabled device which will expect the fragment id.
> > > > > > > > > >>>>
> > > > > > > > > >>>> CC: Eric Dumazet <edumazet@google.com>
> > > > > > > > > >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> > > > > > > > > >>>> ---
> > > > > > > > > >>>> include/linux/skbuff.h | 3 ++-
> > > > > > > > > >>>> include/net/ipv6.h | 2 ++
> > > > > > > > > >>>> net/ipv6/ip6_output.c | 4 ++--
> > > > > > > > > >>>> net/ipv6/output_core.c | 9 ++++++++-
> > > > > > > > > >>>> net/ipv6/udp_offload.c | 10 +++++++++-
> > > > > > > > > >>>> 5 files changed, 23 insertions(+), 5 deletions(-)
> > > > > > > > > >>>>
> > > > > > > > > >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > > > > > > > >>>> index 85ab7d7..3ad5203 100644
> > > > > > > > > >>>> --- a/include/linux/skbuff.h
> > > > > > > > > >>>> +++ b/include/linux/skbuff.h
> > > > > > > > > >>>> @@ -605,7 +605,8 @@ struct sk_buff {
> > > > > > > > > >>>> __u8 ipvs_property:1;
> > > > > > > > > >>>> __u8 inner_protocol_type:1;
> > > > > > > > > >>>> __u8 remcsum_offload:1;
> > > > > > > > > >>>> - /* 3 or 5 bit hole */
> > > > > > > > > >>>> + __u8 ufo_fragid_set:1;
> > > > > > > > > >>> [...]
> > > > > > > > > >>>
> > > > > > > > > >>> Doesn't the flag belong in struct skb_shared_info, rather than struct
> > > > > > > > > >>> sk_buff? Otherwise this looks fine.
> > > > > > > > > >>>
> > > > > > > > > >>> Ben.
> > > > > > > > > >>
> > > > > > > > > >> Hmm we seem to be out of tx flags.
> > > > > > > > > >> Maybe ip6_frag_id == 0 should mean "not set".
> > > > > > > > > >
> > > > > > > > > > Maybe that is the best idea. Definitely the ufo_fragid_set bit should
> > > > > > > > > > move into the skb_shared_info area.
> > > > > > > > >
> > > > > > > > > That's what I originally wanted to do, but had to move and grow txflags thus
> > > > > > > > > skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
> > > > > > > > >
> > > > > > > > > I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
> > > > > > > > > from the protocol perspective and could actually be generated by the id generator
> > > > > > > > > functions. This may cause us to call the id generation multiple times.
> > > > > > > >
> > > > > > > > Are there plans in the long run to let virtio_net transmit auxiliary
> > > > > > > > data to the other end so we can clean all of this this up one day?
> > > > > > > >
> > > > > > > > I don't like the whole situation: looking into the virtio_net headers
> > > > > > > > just adding a field for ipv6 fragmentation ids to those small structs
> > > > > > > > seems bloated, not doing it feels incorrect. :/
> > > > > > > >
> > > > > > > > Thoughts?
> > > > > > > >
> > > > > > > > Bye,
> > > > > > > > Hannes
> > > > > > >
> > > > > > > I'm not sure - what will be achieved by generating the IDs guest side as
> > > > > > > opposed to host side? It's certainly harder to get hold of entropy
> > > > > > > guest-side.
> > > > > >
> > > > > > It is not only about entropy but about uniqueness. Also fragmentation
> > > > > > ids should not be discoverable,
> > > > >
> > > > > I belive "predictable" is the language used by the IETF draft.
> > > > >
> > > > > > so there are several aspects:
> > > > > >
> > > > > > I see fragmentation id generation still as security critical:
> > > > > > When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> > > > > > identifiers less predictable") I could patch my kernels and use the
> > > > > > patch regardless of the machine being virtualized or not. It was not
> > > > > > dependent on the hypervisor.
> > > > >
> > > > > And now it's even easier - just patch the hypervisor, and all VMs
> > > > > automatically benefit.
> > > >
> > > > Sometimes the hypervisor is not under my control.
> > >
> > > In that case doing things like extending virtio
> > > is out of the question too, isn't it?
> > > It needs hypervisor changes.
> >
> > Sure, but I would like to have the fragmentation id generator to reside
> > inside the end-host kernel. Hypervisor needs to carry the frag id along,
> > sure, and needs to be changed accordingly.
> >
> > So in either case we need to change both kernels. ;)
> >
> > >
> > > > You would need to
> > > > patch both kernels in your case - non gso frames would still get the
> > > > fragmentation id generated in the host kernel.
> > > >
> > > > > > I think that is the same reasoning why we
> > > > > > don't support TOE.
> > > > > > If we use one generator in the hypervisor in an openstack alike setting,
> > > > > > the host deals with quite a lot of overlay networks. A lot of default
> > > > > > configurations use the same addresses internally, so on the hypervisor
> > > > > > the frag id generators would interfere by design.
> > > > > > I could come up with an attack scenario for DNS servers (again :) ):
> > > > > >
> > > > > > You are sitting next to a DNS server on the same hypervisor and can send
> > > > > > packets without source validation (because that is handled later on in
> > > > > > case of openvswitch when the packet is put into the corresponding
> > > > > > overlay network). You emit a gso packet with the same source and
> > > > > > destination addresses as the DNS server would do and would get an
> > > > > > fragmentation id which is linearly (+ time delta) incremented depending
> > > > > > on the source and destination address. With such a leak you could start
> > > > > > trying attack and spoof DNS responses (fragmentation attacks etc.).
> > > > > > See also details on such kind of attacks in the description of commit
> > > > > > 04ca6973f7c1a0d.
> > > > > >
> > > > > > AFAIK IETF tried with IPv6 to push fragmentation id generation to the
> > > > > > end hosts, that's also the reason for the introduction of atomic
> > > > > > fragments (which are now being rolled back ;) ).
> > > > > >
> > > > > > Still it is better to generate a frag id on the hypervisor than just
> > > > > > sending a 0, so I am ok with this change, albeit not happy.
> > > > > >
> > > > > > Thanks,
> > > > > > Hannes
> > > > > >
> > > > >
> > > > > OK so to summarize, identifiers are only re-randomized once per jiffy,
> > > > > so you worry that within this window, an external observer can discover
> > > > > past fragment ID values and so predict the future ones.
> > > > > All that's required is that two paths go through the same box performing
> > > > > fragmentation.
> > > > >
> > > > > Is that a fair summary?
> > >
> > > No answer here?
> >
> > Ups, sorry.
> >
> > It is not re-randomized but only biased by a time delta (note the
> > prandom_u32_max). So even after such an increment happens you can still
> > guess the range of the current fragmentation ids for a longer time.
> >
> > Otherwise it is a fair summary.
> >
> > >
> > > > > If yes, we can make this a bit harder by mixing in some
> > > > > data per input and/or output devices.
> > > > >
> > > > > For example, just to give you the idea:
> > > > >
> > > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > > index 683d493..4faa7ef 100644
> > > > > --- a/net/core/dev.c
> > > > > +++ b/net/core/dev.c
> > > > > @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> > > > > trace_netif_receive_skb(skb);
> > > > >
> > > > > orig_dev = skb->dev;
> > > > > + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
> > > > >
> > > > > skb_reset_network_header(skb);
> > > > > if (!skb_transport_header_was_set(skb))
> > > > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > > > > index ce69a12..819a821 100644
> > > > > --- a/net/ipv6/ip6_output.c
> > > > > +++ b/net/ipv6/ip6_output.c
> > > > > @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> > > > > sizeof(struct frag_hdr)) & ~7;
> > > > > skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > > > > ipv6_select_ident(&fhdr, rt);
> > > > > - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> > > > > + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> > > > > + fhdr.identification);
> > > > >
> > > > > append:
> > > > > return skb_append_datato_frags(sk, skb, getfrag, from,
> > > > >
> > > >
> > > > I thought about mixing in the incoming interface identifier into the
> > > > frag id generation, but that could hurt us badly as soon as a VM has
> > > > more than one interface to the outside world and uses e.g. ECMP.
> > > > We need
> > > > to make sure that those frag ids are unique and the kernel needs to be
> > > > better than just using a random number generator.
> > > >
> > > > Bye,
> > > > Hannes
> > >
> > > OK then. Like this:
> > >
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index 679e6e9..1ee9a3a 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -1508,6 +1508,9 @@ struct net_device {
> > > * part of the usual set specified in Space.c.
> > > */
> > >
> > > + /* Extra hash to mix into IPv6 frag ID on packets received from here. */
> > > + unsigned int frag_id_hash;
> > > +
> > > unsigned long state;
> > >
> > > struct list_head dev_list;
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 683d493..56f1898 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> > > trace_netif_receive_skb(skb);
> > >
> > > orig_dev = skb->dev;
> > > + skb_shinfo(skb)->ip6_frag_id = skb->dev->frag_id_hash;
> > >
> > > skb_reset_network_header(skb);
> > > if (!skb_transport_header_was_set(skb))
> > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > > index ce69a12..819a821 100644
> > > --- a/net/ipv6/ip6_output.c
> > > +++ b/net/ipv6/ip6_output.c
> > > @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> > > sizeof(struct frag_hdr)) & ~7;
> > > skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > > ipv6_select_ident(&fhdr, rt);
> > > - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> > > + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> > > + fhdr.identification);
> > >
> > > append:
> > > return skb_append_datato_frags(sk, skb, getfrag, from,
> > >
> > >
> > > Add to this a netlink/sysfs API to set the frag_id_hash for
> > > devices.
> > >
> > > Now, user can set identical frag id hash for all devices
> > > for a given VM.
> > >
> > > We can even expose this to guests: each guest would generate
> > > the ID on boot and send it to host, host would set it
> > > in sysfs.
> >
> > jhash_1word shouldn't be a bijection, so we are randomizing here and are
> > increasing the probability of collisions.
> > Instead of jhash_1word you
> > would need to take a simple block cipher with the hash as key.
> >
> > Bye,
> > Hannes
>
> fhdr.identification is coming from jhash_3word itself, how is this
> different?
>
Sorry, I currently cannot follow. Does it? We hash the ipv6 addresses
and the hash is used as an index into the ip_idents array.
Sorry, maybe I have overlooked something?
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Ben Hutchings @ 2015-01-28 17:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Vladislav Yasevich, virtualization, edumazet,
Hannes Frederic Sowa
In-Reply-To: <20150128094658.GB16775@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 1094 bytes --]
On Wed, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
[...]
> > I see fragmentation id generation still as security critical:
> > When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> > identifiers less predictable") I could patch my kernels and use the
> > patch regardless of the machine being virtualized or not. It was not
> > dependent on the hypervisor.
>
> And now it's even easier - just patch the hypervisor, and all VMs
> automatically benefit.
[...]
You are advocating that the hypervisor should continue to act as a
middle-box that quietly modifies packets. This may be useful to protect
guests that have poor fragment ID generation, but then that should be an
optional netfilter module and *not* the default. The default should be
that UFO has no effect on the packet headers on the wire, and therefore
that the fragment ID is chosen by the IPv6 stack in the guest.
Ben.
--
Ben Hutchings
Teamwork is essential - it allows you to blame someone else.
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v8 0/4] net: Add Keystone NetCP ethernet driver support
From: Murali Karicheri @ 2015-01-28 16:49 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David Miller, devicetree, linux-kernel, netdev
In-Reply-To: <25883654.BLFxICW9P0@wuerfel>
On 01/27/2015 05:28 PM, Arnd Bergmann wrote:
> On Tuesday 20 January 2015 10:53:36 Murali Karicheri wrote:
>> On 01/19/2015 03:11 PM, David Miller wrote:
>>> From: Murali Karicheri<m-karicheri2@ti.com>
>>> Date: Thu, 15 Jan 2015 19:10:03 -0500
>>>
>>>> The Network Coprocessor (NetCP) is a hardware accelerator that processes
>>>> Ethernet packets. NetCP has a gigabit Ethernet (GbE) subsystem with a ethernet
>>>> switch sub-module to send and receive packets. NetCP also includes a packet
>>>> accelerator (PA) module to perform packet classification operations such as
>>>> header matching, and packet modification operations such as checksum
>>>> generation. NetCP can also optionally include a Security Accelerator(SA)
>>>> capable of performing IPSec operations on ingress/egress packets.
>>>>
>>>> Keystone SoC's also have a 10 Gigabit Ethernet Subsystem (XGbE) which
>>>> includes a 3-port Ethernet switch sub-module capable of 10Gb/s and
>>>> 1Gb/s rates per Ethernet port.
>>>>
>>>> Both GBE and XGBE network processors supported using common driver. It
>>>> is also designed to handle future variants of NetCP.
>>>
>>> Series applied to net-next, thanks.
>> David,
>>
>> Thanks a lot for applying this series. This helps us move forward to
>> work on the next set of patches.
>
> Hi Murali,
>
> Building an ARM 'allmodconfig' kernel now runs into two separate problems
> from your driver:
>
> - you have two module_init() instances in one module, which conflict.
>
> - you have two files that are linked into more than one module, so building
> both TI_CPSW and TI_KEYSTONE_NETCP in the same kernel fails.
>
> The answer to both of these is probably to have separate loadable modules,
> but you might be able to come up with a different solution.
Arnd,
Thanks for letting us know. We will look into this.
How do I reproduce this? Is there a defconfig used for allmodconfig? I
am unable to find one. Any details to reproduce this will be useful.
Thanks.
Murali
>
> Arnd
--
Murali Karicheri
Linux Kernel, Texas Instruments
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Michael S. Tsirkin @ 2015-01-28 16:48 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: netdev, Vladislav Yasevich, virtualization, edumazet,
Ben Hutchings
In-Reply-To: <1422461749.4678.110.camel@stressinduktion.org>
On Wed, Jan 28, 2015 at 05:15:49PM +0100, Hannes Frederic Sowa wrote:
> Hi,
>
> On Mi, 2015-01-28 at 18:00 +0200, Michael S. Tsirkin wrote:
> > On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote:
> > > Hi,
> > >
> > > On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
> > > > > Hello,
> > > > >
> > > > > On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
> > > > > > On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
> > > > > > > On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
> > > > > > > > On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
> > > > > > > > > On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
> > > > > > > > >> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
> > > > > > > > >>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
> > > > > > > > >>>> If the IPv6 fragment id has not been set and we perform
> > > > > > > > >>>> fragmentation due to UFO, select a new fragment id.
> > > > > > > > >>>> When we store the fragment id into skb_shinfo, set the bit
> > > > > > > > >>>> in the skb so we can re-use the selected id.
> > > > > > > > >>>> This preserves the behavior of UFO packets generated on the
> > > > > > > > >>>> host and solves the issue of id generation for packet sockets
> > > > > > > > >>>> and tap/macvtap devices.
> > > > > > > > >>>>
> > > > > > > > >>>> This patch moves ipv6_select_ident() back in to the header file.
> > > > > > > > >>>> It also provides the helper function that sets skb_shinfo() frag
> > > > > > > > >>>> id and sets the bit.
> > > > > > > > >>>>
> > > > > > > > >>>> It also makes sure that we select the fragment id when doing
> > > > > > > > >>>> just gso validation, since it's possible for the packet to
> > > > > > > > >>>> come from an untrusted source (VM) and be forwarded through
> > > > > > > > >>>> a UFO enabled device which will expect the fragment id.
> > > > > > > > >>>>
> > > > > > > > >>>> CC: Eric Dumazet <edumazet@google.com>
> > > > > > > > >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> > > > > > > > >>>> ---
> > > > > > > > >>>> include/linux/skbuff.h | 3 ++-
> > > > > > > > >>>> include/net/ipv6.h | 2 ++
> > > > > > > > >>>> net/ipv6/ip6_output.c | 4 ++--
> > > > > > > > >>>> net/ipv6/output_core.c | 9 ++++++++-
> > > > > > > > >>>> net/ipv6/udp_offload.c | 10 +++++++++-
> > > > > > > > >>>> 5 files changed, 23 insertions(+), 5 deletions(-)
> > > > > > > > >>>>
> > > > > > > > >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > > > > > > >>>> index 85ab7d7..3ad5203 100644
> > > > > > > > >>>> --- a/include/linux/skbuff.h
> > > > > > > > >>>> +++ b/include/linux/skbuff.h
> > > > > > > > >>>> @@ -605,7 +605,8 @@ struct sk_buff {
> > > > > > > > >>>> __u8 ipvs_property:1;
> > > > > > > > >>>> __u8 inner_protocol_type:1;
> > > > > > > > >>>> __u8 remcsum_offload:1;
> > > > > > > > >>>> - /* 3 or 5 bit hole */
> > > > > > > > >>>> + __u8 ufo_fragid_set:1;
> > > > > > > > >>> [...]
> > > > > > > > >>>
> > > > > > > > >>> Doesn't the flag belong in struct skb_shared_info, rather than struct
> > > > > > > > >>> sk_buff? Otherwise this looks fine.
> > > > > > > > >>>
> > > > > > > > >>> Ben.
> > > > > > > > >>
> > > > > > > > >> Hmm we seem to be out of tx flags.
> > > > > > > > >> Maybe ip6_frag_id == 0 should mean "not set".
> > > > > > > > >
> > > > > > > > > Maybe that is the best idea. Definitely the ufo_fragid_set bit should
> > > > > > > > > move into the skb_shared_info area.
> > > > > > > >
> > > > > > > > That's what I originally wanted to do, but had to move and grow txflags thus
> > > > > > > > skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
> > > > > > > >
> > > > > > > > I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
> > > > > > > > from the protocol perspective and could actually be generated by the id generator
> > > > > > > > functions. This may cause us to call the id generation multiple times.
> > > > > > >
> > > > > > > Are there plans in the long run to let virtio_net transmit auxiliary
> > > > > > > data to the other end so we can clean all of this this up one day?
> > > > > > >
> > > > > > > I don't like the whole situation: looking into the virtio_net headers
> > > > > > > just adding a field for ipv6 fragmentation ids to those small structs
> > > > > > > seems bloated, not doing it feels incorrect. :/
> > > > > > >
> > > > > > > Thoughts?
> > > > > > >
> > > > > > > Bye,
> > > > > > > Hannes
> > > > > >
> > > > > > I'm not sure - what will be achieved by generating the IDs guest side as
> > > > > > opposed to host side? It's certainly harder to get hold of entropy
> > > > > > guest-side.
> > > > >
> > > > > It is not only about entropy but about uniqueness. Also fragmentation
> > > > > ids should not be discoverable,
> > > >
> > > > I belive "predictable" is the language used by the IETF draft.
> > > >
> > > > > so there are several aspects:
> > > > >
> > > > > I see fragmentation id generation still as security critical:
> > > > > When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> > > > > identifiers less predictable") I could patch my kernels and use the
> > > > > patch regardless of the machine being virtualized or not. It was not
> > > > > dependent on the hypervisor.
> > > >
> > > > And now it's even easier - just patch the hypervisor, and all VMs
> > > > automatically benefit.
> > >
> > > Sometimes the hypervisor is not under my control.
> >
> > In that case doing things like extending virtio
> > is out of the question too, isn't it?
> > It needs hypervisor changes.
>
> Sure, but I would like to have the fragmentation id generator to reside
> inside the end-host kernel. Hypervisor needs to carry the frag id along,
> sure, and needs to be changed accordingly.
>
> So in either case we need to change both kernels. ;)
>
> >
> > > You would need to
> > > patch both kernels in your case - non gso frames would still get the
> > > fragmentation id generated in the host kernel.
> > >
> > > > > I think that is the same reasoning why we
> > > > > don't support TOE.
> > > > > If we use one generator in the hypervisor in an openstack alike setting,
> > > > > the host deals with quite a lot of overlay networks. A lot of default
> > > > > configurations use the same addresses internally, so on the hypervisor
> > > > > the frag id generators would interfere by design.
> > > > > I could come up with an attack scenario for DNS servers (again :) ):
> > > > >
> > > > > You are sitting next to a DNS server on the same hypervisor and can send
> > > > > packets without source validation (because that is handled later on in
> > > > > case of openvswitch when the packet is put into the corresponding
> > > > > overlay network). You emit a gso packet with the same source and
> > > > > destination addresses as the DNS server would do and would get an
> > > > > fragmentation id which is linearly (+ time delta) incremented depending
> > > > > on the source and destination address. With such a leak you could start
> > > > > trying attack and spoof DNS responses (fragmentation attacks etc.).
> > > > > See also details on such kind of attacks in the description of commit
> > > > > 04ca6973f7c1a0d.
> > > > >
> > > > > AFAIK IETF tried with IPv6 to push fragmentation id generation to the
> > > > > end hosts, that's also the reason for the introduction of atomic
> > > > > fragments (which are now being rolled back ;) ).
> > > > >
> > > > > Still it is better to generate a frag id on the hypervisor than just
> > > > > sending a 0, so I am ok with this change, albeit not happy.
> > > > >
> > > > > Thanks,
> > > > > Hannes
> > > > >
> > > >
> > > > OK so to summarize, identifiers are only re-randomized once per jiffy,
> > > > so you worry that within this window, an external observer can discover
> > > > past fragment ID values and so predict the future ones.
> > > > All that's required is that two paths go through the same box performing
> > > > fragmentation.
> > > >
> > > > Is that a fair summary?
> >
> > No answer here?
>
> Ups, sorry.
>
> It is not re-randomized but only biased by a time delta (note the
> prandom_u32_max). So even after such an increment happens you can still
> guess the range of the current fragmentation ids for a longer time.
>
> Otherwise it is a fair summary.
>
> >
> > > > If yes, we can make this a bit harder by mixing in some
> > > > data per input and/or output devices.
> > > >
> > > > For example, just to give you the idea:
> > > >
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index 683d493..4faa7ef 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> > > > trace_netif_receive_skb(skb);
> > > >
> > > > orig_dev = skb->dev;
> > > > + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
> > > >
> > > > skb_reset_network_header(skb);
> > > > if (!skb_transport_header_was_set(skb))
> > > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > > > index ce69a12..819a821 100644
> > > > --- a/net/ipv6/ip6_output.c
> > > > +++ b/net/ipv6/ip6_output.c
> > > > @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> > > > sizeof(struct frag_hdr)) & ~7;
> > > > skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > > > ipv6_select_ident(&fhdr, rt);
> > > > - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> > > > + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> > > > + fhdr.identification);
> > > >
> > > > append:
> > > > return skb_append_datato_frags(sk, skb, getfrag, from,
> > > >
> > >
> > > I thought about mixing in the incoming interface identifier into the
> > > frag id generation, but that could hurt us badly as soon as a VM has
> > > more than one interface to the outside world and uses e.g. ECMP.
> > > We need
> > > to make sure that those frag ids are unique and the kernel needs to be
> > > better than just using a random number generator.
> > >
> > > Bye,
> > > Hannes
> >
> > OK then. Like this:
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 679e6e9..1ee9a3a 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1508,6 +1508,9 @@ struct net_device {
> > * part of the usual set specified in Space.c.
> > */
> >
> > + /* Extra hash to mix into IPv6 frag ID on packets received from here. */
> > + unsigned int frag_id_hash;
> > +
> > unsigned long state;
> >
> > struct list_head dev_list;
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 683d493..56f1898 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> > trace_netif_receive_skb(skb);
> >
> > orig_dev = skb->dev;
> > + skb_shinfo(skb)->ip6_frag_id = skb->dev->frag_id_hash;
> >
> > skb_reset_network_header(skb);
> > if (!skb_transport_header_was_set(skb))
> > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > index ce69a12..819a821 100644
> > --- a/net/ipv6/ip6_output.c
> > +++ b/net/ipv6/ip6_output.c
> > @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> > sizeof(struct frag_hdr)) & ~7;
> > skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > ipv6_select_ident(&fhdr, rt);
> > - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> > + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> > + fhdr.identification);
> >
> > append:
> > return skb_append_datato_frags(sk, skb, getfrag, from,
> >
> >
> > Add to this a netlink/sysfs API to set the frag_id_hash for
> > devices.
> >
> > Now, user can set identical frag id hash for all devices
> > for a given VM.
> >
> > We can even expose this to guests: each guest would generate
> > the ID on boot and send it to host, host would set it
> > in sysfs.
>
> jhash_1word shouldn't be a bijection, so we are randomizing here and are
> increasing the probability of collisions.
> Instead of jhash_1word you
> would need to take a simple block cipher with the hash as key.
>
> Bye,
> Hannes
fhdr.identification is coming from jhash_3word itself, how is this
different?
^ permalink raw reply
* Re: [PATCH v2 linux-trace 4/8] samples: bpf: simple tracing example in C
From: Alexei Starovoitov @ 2015-01-28 16:42 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Steven Rostedt, Ingo Molnar, Namhyung Kim, Jiri Olsa,
Masami Hiramatsu, Linux API, Network Development, LKML
In-Reply-To: <20150128162557.GP7220-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Wed, Jan 28, 2015 at 8:25 AM, Arnaldo Carvalho de Melo
<acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Em Wed, Jan 28, 2015 at 01:24:15PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Jan 27, 2015 at 08:06:09PM -0800, Alexei Starovoitov escreveu:
>> > diff --git a/samples/bpf/tracex1_kern.c b/samples/bpf/tracex1_kern.c
>> > new file mode 100644
>> > index 000000000000..7849ceb4bce6
>> > --- /dev/null
>> > +++ b/samples/bpf/tracex1_kern.c
>> > @@ -0,0 +1,28 @@
>> > +#include <linux/skbuff.h>
>> > +#include <linux/netdevice.h>
>> > +#include <uapi/linux/bpf.h>
>> > +#include <trace/bpf_trace.h>
>> > +#include "bpf_helpers.h"
>> > +
>> > +SEC("events/net/netif_receive_skb")
>> > +int bpf_prog1(struct bpf_context *ctx)
>> > +{
>> > + /*
>> > + * attaches to /sys/kernel/debug/tracing/events/net/netif_receive_skb
>> > + * prints events for loobpack device only
>> > + */
>> > + char devname[] = "lo";
>> > + struct net_device *dev;
>> > + struct sk_buff *skb = 0;
>> > +
>> > + skb = (struct sk_buff *) ctx->arg1;
>> > + dev = bpf_fetch_ptr(&skb->dev);
>> > + if (bpf_memcmp(dev->name, devname, 2) == 0)
>>
>> I'm only starting to look at all this, so bear with me... But why do we
>> need to have it as "bpf_memcmp"? Can't we simply use it as "memcmp" and
>> have it use the right function?
>>
>> Less typing, perhaps we would need to have a:
>>
>> #define memcmp bpf_memcmp(s1, s2, n) bpf_memcmp(s1, s2, n)
>
> Argh, like this:
>
> #define memcmp(s1, s2, n) bpf_memcmp(s1, s2, n)
>
>> in bpf_helpers.h to have it work?
yes, that will work just fine.
Since it's an example I made it explicit that bpf_memcmp()
has memcmp() semantics, but little bit different:
int bpf_memcmp(void *unsafe_ptr, void *safe_ptr, int size)
meaning that one of the pointers can point anywhere and
the function will be doing probe_kernel_read() underneath
similar to bpf_fetch_*() helpers.
If it was plain memcmp() it would give a wrong impression
that vanilla memcmp() can be used.
In general the programs cannot use any library functions
outside of helpers defined in uapi/linux/bpf.h
bpf_fetch_*() helpers are also explicit in examples.
If one need to do a lot of pointer walking, then macro like
#define D(P) ((typeof(P))bpf_fetch_ptr(&P))
would be easier to use: p = D(D(skb->dev)->ifalias)
multiple pointer derefs would look more natural...
^ permalink raw reply
* Re: [PATCH net-next v1 01/18] net: ethtool: propagate get_settings error
From: Ben Hutchings @ 2015-01-28 16:30 UTC (permalink / raw)
To: David Decotigny
Cc: David S. Miller, Amir Vadai, linux-kernel, netdev, linux-api,
Eric Dumazet, Eugenia Emantayev, Or Gerlitz, Ido Shamay,
Joe Perches, Saeed Mahameed, Govindarajulu Varadarajan,
Venkata Duvvuru, Jeff Kirsher, Eyal Perry, Pravin B Shelar,
Ed Swierk, David Decotigny
In-Reply-To: <1422322574-6188-2-git-send-email-ddecotig@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 942 bytes --]
On Mon, 2015-01-26 at 17:35 -0800, David Decotigny wrote:
> From: David Decotigny <decot@googlers.com>
>
> Signed-off-by: David Decotigny <decot@googlers.com>
> ---
> net/core/ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 91f74f3..52efb7e 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -364,7 +364,7 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
>
> if (copy_to_user(useraddr, &cmd, sizeof(cmd)))
> return -EFAULT;
> - return 0;
> + return err;
err cannnot be negative at this point (and if it's positive, that's a
bug in the get_settings implementation which we happen to fix up).
Ben.
> }
>
> static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
--
Ben Hutchings
Teamwork is essential - it allows you to blame someone else.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH v2 linux-trace 4/8] samples: bpf: simple tracing example in C
From: Arnaldo Carvalho de Melo @ 2015-01-28 16:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Steven Rostedt, Ingo Molnar, Namhyung Kim, Jiri Olsa,
Masami Hiramatsu, linux-api, netdev, linux-kernel
In-Reply-To: <20150128162415.GO7220@kernel.org>
Em Wed, Jan 28, 2015 at 01:24:15PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jan 27, 2015 at 08:06:09PM -0800, Alexei Starovoitov escreveu:
> > diff --git a/samples/bpf/tracex1_kern.c b/samples/bpf/tracex1_kern.c
> > new file mode 100644
> > index 000000000000..7849ceb4bce6
> > --- /dev/null
> > +++ b/samples/bpf/tracex1_kern.c
> > @@ -0,0 +1,28 @@
> > +#include <linux/skbuff.h>
> > +#include <linux/netdevice.h>
> > +#include <uapi/linux/bpf.h>
> > +#include <trace/bpf_trace.h>
> > +#include "bpf_helpers.h"
> > +
> > +SEC("events/net/netif_receive_skb")
> > +int bpf_prog1(struct bpf_context *ctx)
> > +{
> > + /*
> > + * attaches to /sys/kernel/debug/tracing/events/net/netif_receive_skb
> > + * prints events for loobpack device only
> > + */
> > + char devname[] = "lo";
> > + struct net_device *dev;
> > + struct sk_buff *skb = 0;
> > +
> > + skb = (struct sk_buff *) ctx->arg1;
> > + dev = bpf_fetch_ptr(&skb->dev);
> > + if (bpf_memcmp(dev->name, devname, 2) == 0)
>
> I'm only starting to look at all this, so bear with me... But why do we
> need to have it as "bpf_memcmp"? Can't we simply use it as "memcmp" and
> have it use the right function?
>
> Less typing, perhaps we would need to have a:
>
> #define memcmp bpf_memcmp(s1, s2, n) bpf_memcmp(s1, s2, n)
Argh, like this:
#define memcmp(s1, s2, n) bpf_memcmp(s1, s2, n)
> in bpf_helpers.h to have it work?
>
> - Arnaldo
>
> > + /* print event using default tracepoint format */
> > + return 1;
> > +
> > + /* drop event */
> > + return 0;
> > +}
^ permalink raw reply
* Re: [PATCH 2/4] net: wan: add missing virt_to_bus dependencies
From: Jan Kasprzak @ 2015-01-28 16:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: netdev, linux-pcmcia, davem, linux-arm-kernel, jeffrey.t.kirsher
In-Reply-To: <1422454504-439085-3-git-send-email-arnd@arndb.de>
Arnd Bergmann wrote:
: The cosa driver is rather outdated and does not get built on most
: platforms because it requires the ISA_DMA_API symbol. However
: there are some ARM platforms that have ISA_DMA_API but no virt_to_bus,
: and they get this build error when enabling the ltpc driver.
:
: drivers/net/wan/cosa.c: In function 'tx_interrupt':
: drivers/net/wan/cosa.c:1768:3: error: implicit declaration of function 'virt_to_bus'
: unsigned long addr = virt_to_bus(cosa->txbuf);
: ^
:
: The same problem exists for the Hostess SV-11 and Sealevel Systems 4021
: drivers.
Hello,
as for COSA, it is OK. Although I would like to know whether there still is
at least one COSA card in use somewhere :-)
Acked-By: Jan "Yenya" Kasprzak <kas@fi.muni.cz>
Thanks,
-Yenya
:
: This adds another dependency in Kconfig to avoid that configuration.
:
: Signed-off-by: Arnd Bergmann <arnd@arndb.de>
: ---
: drivers/net/wan/Kconfig | 6 +++---
: 1 file changed, 3 insertions(+), 3 deletions(-)
:
: diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
: index 94e234975c61..a2fdd15f285a 100644
: --- a/drivers/net/wan/Kconfig
: +++ b/drivers/net/wan/Kconfig
: @@ -25,7 +25,7 @@ if WAN
: # There is no way to detect a comtrol sv11 - force it modular for now.
: config HOSTESS_SV11
: tristate "Comtrol Hostess SV-11 support"
: - depends on ISA && m && ISA_DMA_API && INET && HDLC
: + depends on ISA && m && ISA_DMA_API && INET && HDLC && VIRT_TO_BUS
: help
: Driver for Comtrol Hostess SV-11 network card which
: operates on low speed synchronous serial links at up to
: @@ -37,7 +37,7 @@ config HOSTESS_SV11
: # The COSA/SRP driver has not been tested as non-modular yet.
: config COSA
: tristate "COSA/SRP sync serial boards support"
: - depends on ISA && m && ISA_DMA_API && HDLC
: + depends on ISA && m && ISA_DMA_API && HDLC && VIRT_TO_BUS
: ---help---
: Driver for COSA and SRP synchronous serial boards.
:
: @@ -87,7 +87,7 @@ config LANMEDIA
: # There is no way to detect a Sealevel board. Force it modular
: config SEALEVEL_4021
: tristate "Sealevel Systems 4021 support"
: - depends on ISA && m && ISA_DMA_API && INET && HDLC
: + depends on ISA && m && ISA_DMA_API && INET && HDLC && VIRT_TO_BUS
: help
: This is a driver for the Sealevel Systems ACB 56 serial I/O adapter.
:
: --
: 2.1.0.rc2
--
| Jan "Yenya" Kasprzak <kas at {fi.muni.cz - work | yenya.net - private}> |
| New GPG 4096R/A45477D5 -- see http://www.fi.muni.cz/~kas/pgp-rollover.txt |
| http://www.fi.muni.cz/~kas/ Journal: http://www.fi.muni.cz/~kas/blog/ |
||| "New and improved" is only really improved if it also takes backwards |||
||| compatibility into account, rather than saying "now everybody must do |||
||| things the new and improved - and different - way" --Linus Torvalds |||
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Hannes Frederic Sowa @ 2015-01-28 16:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Vladislav Yasevich, virtualization, edumazet,
Ben Hutchings
In-Reply-To: <20150128160054.GB32439@redhat.com>
Hi,
On Mi, 2015-01-28 at 18:00 +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote:
> > Hi,
> >
> > On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> > > On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
> > > > Hello,
> > > >
> > > > On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
> > > > > On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
> > > > > > On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
> > > > > > > On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
> > > > > > > > On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
> > > > > > > >> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
> > > > > > > >>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
> > > > > > > >>>> If the IPv6 fragment id has not been set and we perform
> > > > > > > >>>> fragmentation due to UFO, select a new fragment id.
> > > > > > > >>>> When we store the fragment id into skb_shinfo, set the bit
> > > > > > > >>>> in the skb so we can re-use the selected id.
> > > > > > > >>>> This preserves the behavior of UFO packets generated on the
> > > > > > > >>>> host and solves the issue of id generation for packet sockets
> > > > > > > >>>> and tap/macvtap devices.
> > > > > > > >>>>
> > > > > > > >>>> This patch moves ipv6_select_ident() back in to the header file.
> > > > > > > >>>> It also provides the helper function that sets skb_shinfo() frag
> > > > > > > >>>> id and sets the bit.
> > > > > > > >>>>
> > > > > > > >>>> It also makes sure that we select the fragment id when doing
> > > > > > > >>>> just gso validation, since it's possible for the packet to
> > > > > > > >>>> come from an untrusted source (VM) and be forwarded through
> > > > > > > >>>> a UFO enabled device which will expect the fragment id.
> > > > > > > >>>>
> > > > > > > >>>> CC: Eric Dumazet <edumazet@google.com>
> > > > > > > >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> > > > > > > >>>> ---
> > > > > > > >>>> include/linux/skbuff.h | 3 ++-
> > > > > > > >>>> include/net/ipv6.h | 2 ++
> > > > > > > >>>> net/ipv6/ip6_output.c | 4 ++--
> > > > > > > >>>> net/ipv6/output_core.c | 9 ++++++++-
> > > > > > > >>>> net/ipv6/udp_offload.c | 10 +++++++++-
> > > > > > > >>>> 5 files changed, 23 insertions(+), 5 deletions(-)
> > > > > > > >>>>
> > > > > > > >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > > > > > >>>> index 85ab7d7..3ad5203 100644
> > > > > > > >>>> --- a/include/linux/skbuff.h
> > > > > > > >>>> +++ b/include/linux/skbuff.h
> > > > > > > >>>> @@ -605,7 +605,8 @@ struct sk_buff {
> > > > > > > >>>> __u8 ipvs_property:1;
> > > > > > > >>>> __u8 inner_protocol_type:1;
> > > > > > > >>>> __u8 remcsum_offload:1;
> > > > > > > >>>> - /* 3 or 5 bit hole */
> > > > > > > >>>> + __u8 ufo_fragid_set:1;
> > > > > > > >>> [...]
> > > > > > > >>>
> > > > > > > >>> Doesn't the flag belong in struct skb_shared_info, rather than struct
> > > > > > > >>> sk_buff? Otherwise this looks fine.
> > > > > > > >>>
> > > > > > > >>> Ben.
> > > > > > > >>
> > > > > > > >> Hmm we seem to be out of tx flags.
> > > > > > > >> Maybe ip6_frag_id == 0 should mean "not set".
> > > > > > > >
> > > > > > > > Maybe that is the best idea. Definitely the ufo_fragid_set bit should
> > > > > > > > move into the skb_shared_info area.
> > > > > > >
> > > > > > > That's what I originally wanted to do, but had to move and grow txflags thus
> > > > > > > skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
> > > > > > >
> > > > > > > I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
> > > > > > > from the protocol perspective and could actually be generated by the id generator
> > > > > > > functions. This may cause us to call the id generation multiple times.
> > > > > >
> > > > > > Are there plans in the long run to let virtio_net transmit auxiliary
> > > > > > data to the other end so we can clean all of this this up one day?
> > > > > >
> > > > > > I don't like the whole situation: looking into the virtio_net headers
> > > > > > just adding a field for ipv6 fragmentation ids to those small structs
> > > > > > seems bloated, not doing it feels incorrect. :/
> > > > > >
> > > > > > Thoughts?
> > > > > >
> > > > > > Bye,
> > > > > > Hannes
> > > > >
> > > > > I'm not sure - what will be achieved by generating the IDs guest side as
> > > > > opposed to host side? It's certainly harder to get hold of entropy
> > > > > guest-side.
> > > >
> > > > It is not only about entropy but about uniqueness. Also fragmentation
> > > > ids should not be discoverable,
> > >
> > > I belive "predictable" is the language used by the IETF draft.
> > >
> > > > so there are several aspects:
> > > >
> > > > I see fragmentation id generation still as security critical:
> > > > When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> > > > identifiers less predictable") I could patch my kernels and use the
> > > > patch regardless of the machine being virtualized or not. It was not
> > > > dependent on the hypervisor.
> > >
> > > And now it's even easier - just patch the hypervisor, and all VMs
> > > automatically benefit.
> >
> > Sometimes the hypervisor is not under my control.
>
> In that case doing things like extending virtio
> is out of the question too, isn't it?
> It needs hypervisor changes.
Sure, but I would like to have the fragmentation id generator to reside
inside the end-host kernel. Hypervisor needs to carry the frag id along,
sure, and needs to be changed accordingly.
So in either case we need to change both kernels. ;)
>
> > You would need to
> > patch both kernels in your case - non gso frames would still get the
> > fragmentation id generated in the host kernel.
> >
> > > > I think that is the same reasoning why we
> > > > don't support TOE.
> > > > If we use one generator in the hypervisor in an openstack alike setting,
> > > > the host deals with quite a lot of overlay networks. A lot of default
> > > > configurations use the same addresses internally, so on the hypervisor
> > > > the frag id generators would interfere by design.
> > > > I could come up with an attack scenario for DNS servers (again :) ):
> > > >
> > > > You are sitting next to a DNS server on the same hypervisor and can send
> > > > packets without source validation (because that is handled later on in
> > > > case of openvswitch when the packet is put into the corresponding
> > > > overlay network). You emit a gso packet with the same source and
> > > > destination addresses as the DNS server would do and would get an
> > > > fragmentation id which is linearly (+ time delta) incremented depending
> > > > on the source and destination address. With such a leak you could start
> > > > trying attack and spoof DNS responses (fragmentation attacks etc.).
> > > > See also details on such kind of attacks in the description of commit
> > > > 04ca6973f7c1a0d.
> > > >
> > > > AFAIK IETF tried with IPv6 to push fragmentation id generation to the
> > > > end hosts, that's also the reason for the introduction of atomic
> > > > fragments (which are now being rolled back ;) ).
> > > >
> > > > Still it is better to generate a frag id on the hypervisor than just
> > > > sending a 0, so I am ok with this change, albeit not happy.
> > > >
> > > > Thanks,
> > > > Hannes
> > > >
> > >
> > > OK so to summarize, identifiers are only re-randomized once per jiffy,
> > > so you worry that within this window, an external observer can discover
> > > past fragment ID values and so predict the future ones.
> > > All that's required is that two paths go through the same box performing
> > > fragmentation.
> > >
> > > Is that a fair summary?
>
> No answer here?
Ups, sorry.
It is not re-randomized but only biased by a time delta (note the
prandom_u32_max). So even after such an increment happens you can still
guess the range of the current fragmentation ids for a longer time.
Otherwise it is a fair summary.
>
> > > If yes, we can make this a bit harder by mixing in some
> > > data per input and/or output devices.
> > >
> > > For example, just to give you the idea:
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 683d493..4faa7ef 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> > > trace_netif_receive_skb(skb);
> > >
> > > orig_dev = skb->dev;
> > > + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
> > >
> > > skb_reset_network_header(skb);
> > > if (!skb_transport_header_was_set(skb))
> > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > > index ce69a12..819a821 100644
> > > --- a/net/ipv6/ip6_output.c
> > > +++ b/net/ipv6/ip6_output.c
> > > @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> > > sizeof(struct frag_hdr)) & ~7;
> > > skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > > ipv6_select_ident(&fhdr, rt);
> > > - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> > > + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> > > + fhdr.identification);
> > >
> > > append:
> > > return skb_append_datato_frags(sk, skb, getfrag, from,
> > >
> >
> > I thought about mixing in the incoming interface identifier into the
> > frag id generation, but that could hurt us badly as soon as a VM has
> > more than one interface to the outside world and uses e.g. ECMP.
> > We need
> > to make sure that those frag ids are unique and the kernel needs to be
> > better than just using a random number generator.
> >
> > Bye,
> > Hannes
>
> OK then. Like this:
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 679e6e9..1ee9a3a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1508,6 +1508,9 @@ struct net_device {
> * part of the usual set specified in Space.c.
> */
>
> + /* Extra hash to mix into IPv6 frag ID on packets received from here. */
> + unsigned int frag_id_hash;
> +
> unsigned long state;
>
> struct list_head dev_list;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 683d493..56f1898 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> trace_netif_receive_skb(skb);
>
> orig_dev = skb->dev;
> + skb_shinfo(skb)->ip6_frag_id = skb->dev->frag_id_hash;
>
> skb_reset_network_header(skb);
> if (!skb_transport_header_was_set(skb))
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index ce69a12..819a821 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> sizeof(struct frag_hdr)) & ~7;
> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> ipv6_select_ident(&fhdr, rt);
> - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> + fhdr.identification);
>
> append:
> return skb_append_datato_frags(sk, skb, getfrag, from,
>
>
> Add to this a netlink/sysfs API to set the frag_id_hash for
> devices.
>
> Now, user can set identical frag id hash for all devices
> for a given VM.
>
> We can even expose this to guests: each guest would generate
> the ID on boot and send it to host, host would set it
> in sysfs.
jhash_1word shouldn't be a bijection, so we are randomizing here and are
increasing the probability of collisions. Instead of jhash_1word you
would need to take a simple block cipher with the hash as key.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Michael S. Tsirkin @ 2015-01-28 16:00 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: netdev, Vladislav Yasevich, virtualization, edumazet,
Ben Hutchings
In-Reply-To: <1422441242.4678.32.camel@stressinduktion.org>
On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote:
> Hi,
>
> On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> > On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
> > > Hello,
> > >
> > > On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
> > > > > On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
> > > > > > On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
> > > > > > > On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
> > > > > > >> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
> > > > > > >>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
> > > > > > >>>> If the IPv6 fragment id has not been set and we perform
> > > > > > >>>> fragmentation due to UFO, select a new fragment id.
> > > > > > >>>> When we store the fragment id into skb_shinfo, set the bit
> > > > > > >>>> in the skb so we can re-use the selected id.
> > > > > > >>>> This preserves the behavior of UFO packets generated on the
> > > > > > >>>> host and solves the issue of id generation for packet sockets
> > > > > > >>>> and tap/macvtap devices.
> > > > > > >>>>
> > > > > > >>>> This patch moves ipv6_select_ident() back in to the header file.
> > > > > > >>>> It also provides the helper function that sets skb_shinfo() frag
> > > > > > >>>> id and sets the bit.
> > > > > > >>>>
> > > > > > >>>> It also makes sure that we select the fragment id when doing
> > > > > > >>>> just gso validation, since it's possible for the packet to
> > > > > > >>>> come from an untrusted source (VM) and be forwarded through
> > > > > > >>>> a UFO enabled device which will expect the fragment id.
> > > > > > >>>>
> > > > > > >>>> CC: Eric Dumazet <edumazet@google.com>
> > > > > > >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> > > > > > >>>> ---
> > > > > > >>>> include/linux/skbuff.h | 3 ++-
> > > > > > >>>> include/net/ipv6.h | 2 ++
> > > > > > >>>> net/ipv6/ip6_output.c | 4 ++--
> > > > > > >>>> net/ipv6/output_core.c | 9 ++++++++-
> > > > > > >>>> net/ipv6/udp_offload.c | 10 +++++++++-
> > > > > > >>>> 5 files changed, 23 insertions(+), 5 deletions(-)
> > > > > > >>>>
> > > > > > >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > > > > >>>> index 85ab7d7..3ad5203 100644
> > > > > > >>>> --- a/include/linux/skbuff.h
> > > > > > >>>> +++ b/include/linux/skbuff.h
> > > > > > >>>> @@ -605,7 +605,8 @@ struct sk_buff {
> > > > > > >>>> __u8 ipvs_property:1;
> > > > > > >>>> __u8 inner_protocol_type:1;
> > > > > > >>>> __u8 remcsum_offload:1;
> > > > > > >>>> - /* 3 or 5 bit hole */
> > > > > > >>>> + __u8 ufo_fragid_set:1;
> > > > > > >>> [...]
> > > > > > >>>
> > > > > > >>> Doesn't the flag belong in struct skb_shared_info, rather than struct
> > > > > > >>> sk_buff? Otherwise this looks fine.
> > > > > > >>>
> > > > > > >>> Ben.
> > > > > > >>
> > > > > > >> Hmm we seem to be out of tx flags.
> > > > > > >> Maybe ip6_frag_id == 0 should mean "not set".
> > > > > > >
> > > > > > > Maybe that is the best idea. Definitely the ufo_fragid_set bit should
> > > > > > > move into the skb_shared_info area.
> > > > > >
> > > > > > That's what I originally wanted to do, but had to move and grow txflags thus
> > > > > > skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
> > > > > >
> > > > > > I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
> > > > > > from the protocol perspective and could actually be generated by the id generator
> > > > > > functions. This may cause us to call the id generation multiple times.
> > > > >
> > > > > Are there plans in the long run to let virtio_net transmit auxiliary
> > > > > data to the other end so we can clean all of this this up one day?
> > > > >
> > > > > I don't like the whole situation: looking into the virtio_net headers
> > > > > just adding a field for ipv6 fragmentation ids to those small structs
> > > > > seems bloated, not doing it feels incorrect. :/
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > Bye,
> > > > > Hannes
> > > >
> > > > I'm not sure - what will be achieved by generating the IDs guest side as
> > > > opposed to host side? It's certainly harder to get hold of entropy
> > > > guest-side.
> > >
> > > It is not only about entropy but about uniqueness. Also fragmentation
> > > ids should not be discoverable,
> >
> > I belive "predictable" is the language used by the IETF draft.
> >
> > > so there are several aspects:
> > >
> > > I see fragmentation id generation still as security critical:
> > > When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> > > identifiers less predictable") I could patch my kernels and use the
> > > patch regardless of the machine being virtualized or not. It was not
> > > dependent on the hypervisor.
> >
> > And now it's even easier - just patch the hypervisor, and all VMs
> > automatically benefit.
>
> Sometimes the hypervisor is not under my control.
In that case doing things like extending virtio
is out of the question too, isn't it?
It needs hypervisor changes.
> You would need to
> patch both kernels in your case - non gso frames would still get the
> fragmentation id generated in the host kernel.
>
> > > I think that is the same reasoning why we
> > > don't support TOE.
> > > If we use one generator in the hypervisor in an openstack alike setting,
> > > the host deals with quite a lot of overlay networks. A lot of default
> > > configurations use the same addresses internally, so on the hypervisor
> > > the frag id generators would interfere by design.
> > > I could come up with an attack scenario for DNS servers (again :) ):
> > >
> > > You are sitting next to a DNS server on the same hypervisor and can send
> > > packets without source validation (because that is handled later on in
> > > case of openvswitch when the packet is put into the corresponding
> > > overlay network). You emit a gso packet with the same source and
> > > destination addresses as the DNS server would do and would get an
> > > fragmentation id which is linearly (+ time delta) incremented depending
> > > on the source and destination address. With such a leak you could start
> > > trying attack and spoof DNS responses (fragmentation attacks etc.).
> > > See also details on such kind of attacks in the description of commit
> > > 04ca6973f7c1a0d.
> > >
> > > AFAIK IETF tried with IPv6 to push fragmentation id generation to the
> > > end hosts, that's also the reason for the introduction of atomic
> > > fragments (which are now being rolled back ;) ).
> > >
> > > Still it is better to generate a frag id on the hypervisor than just
> > > sending a 0, so I am ok with this change, albeit not happy.
> > >
> > > Thanks,
> > > Hannes
> > >
> >
> > OK so to summarize, identifiers are only re-randomized once per jiffy,
> > so you worry that within this window, an external observer can discover
> > past fragment ID values and so predict the future ones.
> > All that's required is that two paths go through the same box performing
> > fragmentation.
> >
> > Is that a fair summary?
No answer here?
> > If yes, we can make this a bit harder by mixing in some
> > data per input and/or output devices.
> >
> > For example, just to give you the idea:
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 683d493..4faa7ef 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> > trace_netif_receive_skb(skb);
> >
> > orig_dev = skb->dev;
> > + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
> >
> > skb_reset_network_header(skb);
> > if (!skb_transport_header_was_set(skb))
> > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > index ce69a12..819a821 100644
> > --- a/net/ipv6/ip6_output.c
> > +++ b/net/ipv6/ip6_output.c
> > @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> > sizeof(struct frag_hdr)) & ~7;
> > skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > ipv6_select_ident(&fhdr, rt);
> > - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> > + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> > + fhdr.identification);
> >
> > append:
> > return skb_append_datato_frags(sk, skb, getfrag, from,
> >
>
> I thought about mixing in the incoming interface identifier into the
> frag id generation, but that could hurt us badly as soon as a VM has
> more than one interface to the outside world and uses e.g. ECMP.
> We need
> to make sure that those frag ids are unique and the kernel needs to be
> better than just using a random number generator.
>
> Bye,
> Hannes
OK then. Like this:
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 679e6e9..1ee9a3a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1508,6 +1508,9 @@ struct net_device {
* part of the usual set specified in Space.c.
*/
+ /* Extra hash to mix into IPv6 frag ID on packets received from here. */
+ unsigned int frag_id_hash;
+
unsigned long state;
struct list_head dev_list;
diff --git a/net/core/dev.c b/net/core/dev.c
index 683d493..56f1898 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
trace_netif_receive_skb(skb);
orig_dev = skb->dev;
+ skb_shinfo(skb)->ip6_frag_id = skb->dev->frag_id_hash;
skb_reset_network_header(skb);
if (!skb_transport_header_was_set(skb))
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index ce69a12..819a821 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
sizeof(struct frag_hdr)) & ~7;
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
ipv6_select_ident(&fhdr, rt);
- skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
+ skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
+ fhdr.identification);
append:
return skb_append_datato_frags(sk, skb, getfrag, from,
Add to this a netlink/sysfs API to set the frag_id_hash for
devices.
Now, user can set identical frag id hash for all devices
for a given VM.
We can even expose this to guests: each guest would generate
the ID on boot and send it to host, host would set it
in sysfs.
--
MST
^ permalink raw reply related
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Michael S. Tsirkin @ 2015-01-28 15:49 UTC (permalink / raw)
To: Vlad Yasevich
Cc: netdev, Vladislav Yasevich, virtualization, edumazet,
Hannes Frederic Sowa, Ben Hutchings
In-Reply-To: <54C8FFF3.7050800@redhat.com>
On Wed, Jan 28, 2015 at 10:27:47AM -0500, Vlad Yasevich wrote:
> On 01/28/2015 09:45 AM, Hannes Frederic Sowa wrote:
> > Hi,
> >
> > On Mi, 2015-01-28 at 09:16 -0500, Vlad Yasevich wrote:
> >> On 01/28/2015 05:34 AM, Hannes Frederic Sowa wrote:
> >>> Hi,
> >>>
> >>> On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> >>>> On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
> >>>>> Hello,
> >>>>>
> >>>>> On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
> >>>>>> On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
> >>>>>>> On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
> >>>>>>>> On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
> >>>>>>>>> On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
> >>>>>>>>>> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
> >>>>>>>>>>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
> >>>>>>>>>>>> If the IPv6 fragment id has not been set and we perform
> >>>>>>>>>>>> fragmentation due to UFO, select a new fragment id.
> >>>>>>>>>>>> When we store the fragment id into skb_shinfo, set the bit
> >>>>>>>>>>>> in the skb so we can re-use the selected id.
> >>>>>>>>>>>> This preserves the behavior of UFO packets generated on the
> >>>>>>>>>>>> host and solves the issue of id generation for packet sockets
> >>>>>>>>>>>> and tap/macvtap devices.
> >>>>>>>>>>>>
> >>>>>>>>>>>> This patch moves ipv6_select_ident() back in to the header file.
> >>>>>>>>>>>> It also provides the helper function that sets skb_shinfo() frag
> >>>>>>>>>>>> id and sets the bit.
> >>>>>>>>>>>>
> >>>>>>>>>>>> It also makes sure that we select the fragment id when doing
> >>>>>>>>>>>> just gso validation, since it's possible for the packet to
> >>>>>>>>>>>> come from an untrusted source (VM) and be forwarded through
> >>>>>>>>>>>> a UFO enabled device which will expect the fragment id.
> >>>>>>>>>>>>
> >>>>>>>>>>>> CC: Eric Dumazet <edumazet@google.com>
> >>>>>>>>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>> include/linux/skbuff.h | 3 ++-
> >>>>>>>>>>>> include/net/ipv6.h | 2 ++
> >>>>>>>>>>>> net/ipv6/ip6_output.c | 4 ++--
> >>>>>>>>>>>> net/ipv6/output_core.c | 9 ++++++++-
> >>>>>>>>>>>> net/ipv6/udp_offload.c | 10 +++++++++-
> >>>>>>>>>>>> 5 files changed, 23 insertions(+), 5 deletions(-)
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >>>>>>>>>>>> index 85ab7d7..3ad5203 100644
> >>>>>>>>>>>> --- a/include/linux/skbuff.h
> >>>>>>>>>>>> +++ b/include/linux/skbuff.h
> >>>>>>>>>>>> @@ -605,7 +605,8 @@ struct sk_buff {
> >>>>>>>>>>>> __u8 ipvs_property:1;
> >>>>>>>>>>>> __u8 inner_protocol_type:1;
> >>>>>>>>>>>> __u8 remcsum_offload:1;
> >>>>>>>>>>>> - /* 3 or 5 bit hole */
> >>>>>>>>>>>> + __u8 ufo_fragid_set:1;
> >>>>>>>>>>> [...]
> >>>>>>>>>>>
> >>>>>>>>>>> Doesn't the flag belong in struct skb_shared_info, rather than struct
> >>>>>>>>>>> sk_buff? Otherwise this looks fine.
> >>>>>>>>>>>
> >>>>>>>>>>> Ben.
> >>>>>>>>>>
> >>>>>>>>>> Hmm we seem to be out of tx flags.
> >>>>>>>>>> Maybe ip6_frag_id == 0 should mean "not set".
> >>>>>>>>>
> >>>>>>>>> Maybe that is the best idea. Definitely the ufo_fragid_set bit should
> >>>>>>>>> move into the skb_shared_info area.
> >>>>>>>>
> >>>>>>>> That's what I originally wanted to do, but had to move and grow txflags thus
> >>>>>>>> skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
> >>>>>>>>
> >>>>>>>> I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
> >>>>>>>> from the protocol perspective and could actually be generated by the id generator
> >>>>>>>> functions. This may cause us to call the id generation multiple times.
> >>>>>>>
> >>>>>>> Are there plans in the long run to let virtio_net transmit auxiliary
> >>>>>>> data to the other end so we can clean all of this this up one day?
> >>>>>>>
> >>>>>>> I don't like the whole situation: looking into the virtio_net headers
> >>>>>>> just adding a field for ipv6 fragmentation ids to those small structs
> >>>>>>> seems bloated, not doing it feels incorrect. :/
> >>>>>>>
> >>>>>>> Thoughts?
> >>>>>>>
> >>>>>>> Bye,
> >>>>>>> Hannes
> >>>>>>
> >>>>>> I'm not sure - what will be achieved by generating the IDs guest side as
> >>>>>> opposed to host side? It's certainly harder to get hold of entropy
> >>>>>> guest-side.
> >>>>>
> >>>>> It is not only about entropy but about uniqueness. Also fragmentation
> >>>>> ids should not be discoverable,
> >>>>
> >>>> I belive "predictable" is the language used by the IETF draft.
> >>>>
> >>>>> so there are several aspects:
> >>>>>
> >>>>> I see fragmentation id generation still as security critical:
> >>>>> When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> >>>>> identifiers less predictable") I could patch my kernels and use the
> >>>>> patch regardless of the machine being virtualized or not. It was not
> >>>>> dependent on the hypervisor.
> >>>>
> >>>> And now it's even easier - just patch the hypervisor, and all VMs
> >>>> automatically benefit.
> >>>
> >>> Sometimes the hypervisor is not under my control. You would need to
> >>> patch both kernels in your case - non gso frames would still get the
> >>> fragmentation id generated in the host kernel.
> >>
> >> Why would non-gso frames need a frag id? We are talking only UDP IPv6
> >> here, so there is no frag id generation if the packet does't need to
> >> be fragmented.
> >
> > E.g. raw sockets still can generate fragments locally. It is also a
> > valid setup to have multiple interfaces in one machine, one that is UFO
> > enabled and one that isn't. In that case, fragmentation id generation
> > happens on different hosts which I want to avoid.
>
> OK, so you are concerned about both host and guest generating fragment
> ids. Host would do it for GSO frames and guest would do it for fragmented
> frames. Yes, there is room for collision,
collision is not a problem. It is in fact unavoidable.
> which is why we are aiming to
> fix this with fragment id passing through virtio_net. However, I am still
> trying to figure the best way to do this as it extends the virtio_net header
> and we want to do it right.
>
> >
> > I haven't looked closely but mismatch of MTUs on interfaces seems like
> > it could lead to unwanted fragmentation, e.g. see is_skb_forwardable
> > which is mostly always true for gso frames, so we never stop them on
> > bridges etc.
>
> Yes, this is one of the cases that gets triggered with VMs.
>
> >
> >>>>> I think that is the same reasoning why we
> >>>>> don't support TOE.
> >>>>> If we use one generator in the hypervisor in an openstack alike setting,
> >>>>> the host deals with quite a lot of overlay networks. A lot of default
> >>>>> configurations use the same addresses internally, so on the hypervisor
> >>>>> the frag id generators would interfere by design.
> >>>>> I could come up with an attack scenario for DNS servers (again :) ):
> >>>>>
> >>>>> You are sitting next to a DNS server on the same hypervisor and can send
> >>>>> packets without source validation (because that is handled later on in
> >>>>> case of openvswitch when the packet is put into the corresponding
> >>>>> overlay network). You emit a gso packet with the same source and
> >>>>> destination addresses as the DNS server would do and would get an
> >>>>> fragmentation id which is linearly (+ time delta) incremented depending
> >>>>> on the source and destination address. With such a leak you could start
> >>>>> trying attack and spoof DNS responses (fragmentation attacks etc.).
> >>>>> See also details on such kind of attacks in the description of commit
> >>>>> 04ca6973f7c1a0d.
> >>>>>
> >>>>> AFAIK IETF tried with IPv6 to push fragmentation id generation to the
> >>>>> end hosts, that's also the reason for the introduction of atomic
> >>>>> fragments (which are now being rolled back ;) ).
> >>>>>
> >>>>> Still it is better to generate a frag id on the hypervisor than just
> >>>>> sending a 0, so I am ok with this change, albeit not happy.
> >>>>>
> >>>>> Thanks,
> >>>>> Hannes
> >>>>>
> >>>>
> >>>> OK so to summarize, identifiers are only re-randomized once per jiffy,
> >>>> so you worry that within this window, an external observer can discover
> >>>> past fragment ID values and so predict the future ones.
> >>>> All that's required is that two paths go through the same box performing
> >>>> fragmentation.
> >>>>
> >>>> Is that a fair summary?
> >>>>
> >>>> If yes, we can make this a bit harder by mixing in some
> >>>> data per input and/or output devices.
> >>>>
> >>>> For example, just to give you the idea:
> >>>>
> >>>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>>> index 683d493..4faa7ef 100644
> >>>> --- a/net/core/dev.c
> >>>> +++ b/net/core/dev.c
> >>>> @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> >>>> trace_netif_receive_skb(skb);
> >>>>
> >>>> orig_dev = skb->dev;
> >>>> + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
> >>>>
> >>>> skb_reset_network_header(skb);
> >>>> if (!skb_transport_header_was_set(skb))
> >>>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> >>>> index ce69a12..819a821 100644
> >>>> --- a/net/ipv6/ip6_output.c
> >>>> +++ b/net/ipv6/ip6_output.c
> >>>> @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> >>>> sizeof(struct frag_hdr)) & ~7;
> >>>> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> >>>> ipv6_select_ident(&fhdr, rt);
> >>>> - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> >>>> + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> >>>> + fhdr.identification);
> >>>>
> >>>> append:
> >>>> return skb_append_datato_frags(sk, skb, getfrag, from,
> >>>>
> >>>
> >>> I thought about mixing in the incoming interface identifier into the
> >>> frag id generation, but that could hurt us badly as soon as a VM has
> >>> more than one interface to the outside world and uses e.g. ECMP. We need
> >>> to make sure that those frag ids are unique and the kernel needs to be
> >>> better than just using a random number generator.
> >>>
> >>
> >> So the goal behind this series of patches is to restore VM functionality to
> >> pre-916e4cf46d0204 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data").
> >
> > I understand (the patch fixed a NULL ptr deref btw.).
> >
> > As I said, I don't want to stop this series (hopefully the flag can be
> > moved into skb_shared_info etc.), would look after that IMHO
> > (skb flags/IPCB and skb_shared_info have different semantics on
> > __skb_clone).
> >
> > I think it is very much worth to try to move the fragmentation id
> > generation back to the end host and only use this as a fallback.
>
> I think we are in agreement here.
>
> -vlad
> >
> > Bye,
> > Hannes
> >
> >
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Vlad Yasevich @ 2015-01-28 15:27 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Michael S. Tsirkin, netdev, Vladislav Yasevich, virtualization,
edumazet, Ben Hutchings
In-Reply-To: <1422456339.4678.85.camel@stressinduktion.org>
On 01/28/2015 09:45 AM, Hannes Frederic Sowa wrote:
> Hi,
>
> On Mi, 2015-01-28 at 09:16 -0500, Vlad Yasevich wrote:
>> On 01/28/2015 05:34 AM, Hannes Frederic Sowa wrote:
>>> Hi,
>>>
>>> On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
>>>> On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
>>>>> Hello,
>>>>>
>>>>> On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
>>>>>> On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
>>>>>>> On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
>>>>>>>> On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
>>>>>>>>> On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
>>>>>>>>>> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
>>>>>>>>>>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
>>>>>>>>>>>> If the IPv6 fragment id has not been set and we perform
>>>>>>>>>>>> fragmentation due to UFO, select a new fragment id.
>>>>>>>>>>>> When we store the fragment id into skb_shinfo, set the bit
>>>>>>>>>>>> in the skb so we can re-use the selected id.
>>>>>>>>>>>> This preserves the behavior of UFO packets generated on the
>>>>>>>>>>>> host and solves the issue of id generation for packet sockets
>>>>>>>>>>>> and tap/macvtap devices.
>>>>>>>>>>>>
>>>>>>>>>>>> This patch moves ipv6_select_ident() back in to the header file.
>>>>>>>>>>>> It also provides the helper function that sets skb_shinfo() frag
>>>>>>>>>>>> id and sets the bit.
>>>>>>>>>>>>
>>>>>>>>>>>> It also makes sure that we select the fragment id when doing
>>>>>>>>>>>> just gso validation, since it's possible for the packet to
>>>>>>>>>>>> come from an untrusted source (VM) and be forwarded through
>>>>>>>>>>>> a UFO enabled device which will expect the fragment id.
>>>>>>>>>>>>
>>>>>>>>>>>> CC: Eric Dumazet <edumazet@google.com>
>>>>>>>>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> include/linux/skbuff.h | 3 ++-
>>>>>>>>>>>> include/net/ipv6.h | 2 ++
>>>>>>>>>>>> net/ipv6/ip6_output.c | 4 ++--
>>>>>>>>>>>> net/ipv6/output_core.c | 9 ++++++++-
>>>>>>>>>>>> net/ipv6/udp_offload.c | 10 +++++++++-
>>>>>>>>>>>> 5 files changed, 23 insertions(+), 5 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>>>>>>>>>> index 85ab7d7..3ad5203 100644
>>>>>>>>>>>> --- a/include/linux/skbuff.h
>>>>>>>>>>>> +++ b/include/linux/skbuff.h
>>>>>>>>>>>> @@ -605,7 +605,8 @@ struct sk_buff {
>>>>>>>>>>>> __u8 ipvs_property:1;
>>>>>>>>>>>> __u8 inner_protocol_type:1;
>>>>>>>>>>>> __u8 remcsum_offload:1;
>>>>>>>>>>>> - /* 3 or 5 bit hole */
>>>>>>>>>>>> + __u8 ufo_fragid_set:1;
>>>>>>>>>>> [...]
>>>>>>>>>>>
>>>>>>>>>>> Doesn't the flag belong in struct skb_shared_info, rather than struct
>>>>>>>>>>> sk_buff? Otherwise this looks fine.
>>>>>>>>>>>
>>>>>>>>>>> Ben.
>>>>>>>>>>
>>>>>>>>>> Hmm we seem to be out of tx flags.
>>>>>>>>>> Maybe ip6_frag_id == 0 should mean "not set".
>>>>>>>>>
>>>>>>>>> Maybe that is the best idea. Definitely the ufo_fragid_set bit should
>>>>>>>>> move into the skb_shared_info area.
>>>>>>>>
>>>>>>>> That's what I originally wanted to do, but had to move and grow txflags thus
>>>>>>>> skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
>>>>>>>>
>>>>>>>> I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
>>>>>>>> from the protocol perspective and could actually be generated by the id generator
>>>>>>>> functions. This may cause us to call the id generation multiple times.
>>>>>>>
>>>>>>> Are there plans in the long run to let virtio_net transmit auxiliary
>>>>>>> data to the other end so we can clean all of this this up one day?
>>>>>>>
>>>>>>> I don't like the whole situation: looking into the virtio_net headers
>>>>>>> just adding a field for ipv6 fragmentation ids to those small structs
>>>>>>> seems bloated, not doing it feels incorrect. :/
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> Bye,
>>>>>>> Hannes
>>>>>>
>>>>>> I'm not sure - what will be achieved by generating the IDs guest side as
>>>>>> opposed to host side? It's certainly harder to get hold of entropy
>>>>>> guest-side.
>>>>>
>>>>> It is not only about entropy but about uniqueness. Also fragmentation
>>>>> ids should not be discoverable,
>>>>
>>>> I belive "predictable" is the language used by the IETF draft.
>>>>
>>>>> so there are several aspects:
>>>>>
>>>>> I see fragmentation id generation still as security critical:
>>>>> When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
>>>>> identifiers less predictable") I could patch my kernels and use the
>>>>> patch regardless of the machine being virtualized or not. It was not
>>>>> dependent on the hypervisor.
>>>>
>>>> And now it's even easier - just patch the hypervisor, and all VMs
>>>> automatically benefit.
>>>
>>> Sometimes the hypervisor is not under my control. You would need to
>>> patch both kernels in your case - non gso frames would still get the
>>> fragmentation id generated in the host kernel.
>>
>> Why would non-gso frames need a frag id? We are talking only UDP IPv6
>> here, so there is no frag id generation if the packet does't need to
>> be fragmented.
>
> E.g. raw sockets still can generate fragments locally. It is also a
> valid setup to have multiple interfaces in one machine, one that is UFO
> enabled and one that isn't. In that case, fragmentation id generation
> happens on different hosts which I want to avoid.
OK, so you are concerned about both host and guest generating fragment
ids. Host would do it for GSO frames and guest would do it for fragmented
frames. Yes, there is room for collision, which is why we are aiming to
fix this with fragment id passing through virtio_net. However, I am still
trying to figure the best way to do this as it extends the virtio_net header
and we want to do it right.
>
> I haven't looked closely but mismatch of MTUs on interfaces seems like
> it could lead to unwanted fragmentation, e.g. see is_skb_forwardable
> which is mostly always true for gso frames, so we never stop them on
> bridges etc.
Yes, this is one of the cases that gets triggered with VMs.
>
>>>>> I think that is the same reasoning why we
>>>>> don't support TOE.
>>>>> If we use one generator in the hypervisor in an openstack alike setting,
>>>>> the host deals with quite a lot of overlay networks. A lot of default
>>>>> configurations use the same addresses internally, so on the hypervisor
>>>>> the frag id generators would interfere by design.
>>>>> I could come up with an attack scenario for DNS servers (again :) ):
>>>>>
>>>>> You are sitting next to a DNS server on the same hypervisor and can send
>>>>> packets without source validation (because that is handled later on in
>>>>> case of openvswitch when the packet is put into the corresponding
>>>>> overlay network). You emit a gso packet with the same source and
>>>>> destination addresses as the DNS server would do and would get an
>>>>> fragmentation id which is linearly (+ time delta) incremented depending
>>>>> on the source and destination address. With such a leak you could start
>>>>> trying attack and spoof DNS responses (fragmentation attacks etc.).
>>>>> See also details on such kind of attacks in the description of commit
>>>>> 04ca6973f7c1a0d.
>>>>>
>>>>> AFAIK IETF tried with IPv6 to push fragmentation id generation to the
>>>>> end hosts, that's also the reason for the introduction of atomic
>>>>> fragments (which are now being rolled back ;) ).
>>>>>
>>>>> Still it is better to generate a frag id on the hypervisor than just
>>>>> sending a 0, so I am ok with this change, albeit not happy.
>>>>>
>>>>> Thanks,
>>>>> Hannes
>>>>>
>>>>
>>>> OK so to summarize, identifiers are only re-randomized once per jiffy,
>>>> so you worry that within this window, an external observer can discover
>>>> past fragment ID values and so predict the future ones.
>>>> All that's required is that two paths go through the same box performing
>>>> fragmentation.
>>>>
>>>> Is that a fair summary?
>>>>
>>>> If yes, we can make this a bit harder by mixing in some
>>>> data per input and/or output devices.
>>>>
>>>> For example, just to give you the idea:
>>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index 683d493..4faa7ef 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
>>>> trace_netif_receive_skb(skb);
>>>>
>>>> orig_dev = skb->dev;
>>>> + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
>>>>
>>>> skb_reset_network_header(skb);
>>>> if (!skb_transport_header_was_set(skb))
>>>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>>>> index ce69a12..819a821 100644
>>>> --- a/net/ipv6/ip6_output.c
>>>> +++ b/net/ipv6/ip6_output.c
>>>> @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
>>>> sizeof(struct frag_hdr)) & ~7;
>>>> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
>>>> ipv6_select_ident(&fhdr, rt);
>>>> - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
>>>> + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
>>>> + fhdr.identification);
>>>>
>>>> append:
>>>> return skb_append_datato_frags(sk, skb, getfrag, from,
>>>>
>>>
>>> I thought about mixing in the incoming interface identifier into the
>>> frag id generation, but that could hurt us badly as soon as a VM has
>>> more than one interface to the outside world and uses e.g. ECMP. We need
>>> to make sure that those frag ids are unique and the kernel needs to be
>>> better than just using a random number generator.
>>>
>>
>> So the goal behind this series of patches is to restore VM functionality to
>> pre-916e4cf46d0204 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data").
>
> I understand (the patch fixed a NULL ptr deref btw.).
>
> As I said, I don't want to stop this series (hopefully the flag can be
> moved into skb_shared_info etc.), would look after that IMHO
> (skb flags/IPCB and skb_shared_info have different semantics on
> __skb_clone).
>
> I think it is very much worth to try to move the fragmentation id
> generation back to the end host and only use this as a fallback.
I think we are in agreement here.
-vlad
>
> Bye,
> Hannes
>
>
^ permalink raw reply
* RE: [PATCH v5] can: Convert to runtime_pm
From: Appana Durga Kedareswara Rao @ 2015-01-28 15:19 UTC (permalink / raw)
To: Marc Kleine-Budde, wg@grandegger.com, Michal Simek,
Soren Brinkmann, grant.likely@linaro.org, robh+dt@kernel.org
Cc: netdev@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-can@vger.kernel.org
In-Reply-To: <54C8F62D.6090909@pengutronix.de>
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, January 28, 2015 8:16 PM
> To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek;
> Soren Brinkmann; grant.likely@linaro.org; robh+dt@kernel.org
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Appana Durga Kedareswara Rao
> Subject: Re: [PATCH v5] can: Convert to runtime_pm
>
> On 01/12/2015 04:04 PM, Kedareswara rao Appana wrote:
> > Instead of enabling/disabling clocks at several locations in the
> > driver, Use the runtime_pm framework. This consolidates the actions
> > for runtime PM In the appropriate callbacks and makes the driver more
> readable and mantainable.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Changes for v5:
> > - Updated with the review comments.
> > Updated the remove fuction to use runtime_pm.
> > Chnages for v4:
> > - Updated with the review comments.
> > Changes for v3:
> > - Converted the driver to use runtime_pm.
> > Changes for v2:
> > - Removed the struct platform_device* from suspend/resume
> > as suggest by Lothar.
>
> Any plans to submit a v6?
I was on vacation till yesterday just came to office today only. Will look into it and will send v6 at the earliest.
Regards,
Kedar.
>
> Marc
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: [PATCH 2/2] string_helpers: Change semantics of string_escape_mem
From: Andy Shevchenko @ 2015-01-28 15:05 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller,
linux-kernel, linux-nfs, netdev
In-Reply-To: <1422451543-12401-3-git-send-email-linux@rasmusvillemoes.dk>
On Wed, 2015-01-28 at 14:25 +0100, Rasmus Villemoes wrote:
> The current semantics of string_escape_mem are inadequate for one of
> its two current users, vsnprintf(). If that is to honour its contract,
> it must know how much space would be needed for the entire escaped
> buffer, and string_escape_mem provides no way of obtaining that (short
> of allocating a large enough buffer (~4 times input string) to let it
> play with, and that's definitely a big no-no inside vsnprintf).
>
> So change the semantics for string_escape_mem to be more
> snprintf-like: Return the size of the output that would be generated
> if the destination buffer was big enough, but of course still only
> write to the part of dst it is allowed to, and don't do
> '\0'-termination. It is then up to the caller to detect whether output
> was truncated and to append a '\0' if desired.
>
> This also fixes a bug in the escaped_string() helper function, which
> used to unconditionally pass a length of "end-buf" to
> string_escape_mem(); since the latter doesn't check osz for being
> insanely large, it would happily write to dst. For example,
> kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way
> to trigger an oops.
> The patch is somewhat larger than I'd like, but I couldn't find a way
> of splitting it into smaller pieces. Implementation-wise, I changed
> the various escape_* helpers to return true if they handled the
> character, updating dst appropriately, false otherwise. Maybe there's
> a more elegant way, but this seems to work.
Can we split this to at least two parts: internal API changes to
string_escape_mem() and the rest?
> In test-string_helpers.c, I removed the now meaningless -ENOMEM test,
> and replaced it with testing for getting the expected return value
> even if the buffer is too small. Also ensure that nothing is written
> when osz==0.
>
> In net/sunrpc/cache.c, I think qword_add still has the same
> semantics. Someone should definitely double-check this.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v5] can: Convert to runtime_pm
From: Marc Kleine-Budde @ 2015-01-28 14:46 UTC (permalink / raw)
To: Kedareswara rao Appana, wg, michal.simek, soren.brinkmann,
grant.likely, robh+dt
Cc: devicetree, netdev, linux-kernel, linux-can,
Kedareswara rao Appana, linux-arm-kernel
In-Reply-To: <baf987c11c0242c2bf87b81f9396b09d@BN1BFFO11FD018.protection.gbl>
[-- Attachment #1.1: Type: text/plain, Size: 1088 bytes --]
On 01/12/2015 04:04 PM, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the driver,
> Use the runtime_pm framework. This consolidates the actions for runtime PM
> In the appropriate callbacks and makes the driver more readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v5:
> - Updated with the review comments.
> Updated the remove fuction to use runtime_pm.
> Chnages for v4:
> - Updated with the review comments.
> Changes for v3:
> - Converted the driver to use runtime_pm.
> Changes for v2:
> - Removed the struct platform_device* from suspend/resume
> as suggest by Lothar.
Any plans to submit a v6?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Hannes Frederic Sowa @ 2015-01-28 14:45 UTC (permalink / raw)
To: vyasevic
Cc: Michael S. Tsirkin, netdev, Vladislav Yasevich, virtualization,
edumazet, Ben Hutchings
In-Reply-To: <54C8EF25.1030302@redhat.com>
Hi,
On Mi, 2015-01-28 at 09:16 -0500, Vlad Yasevich wrote:
> On 01/28/2015 05:34 AM, Hannes Frederic Sowa wrote:
> > Hi,
> >
> > On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> >> On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
> >>> Hello,
> >>>
> >>> On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
> >>>> On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
> >>>>> On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
> >>>>>> On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
> >>>>>>> On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
> >>>>>>>> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
> >>>>>>>>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
> >>>>>>>>>> If the IPv6 fragment id has not been set and we perform
> >>>>>>>>>> fragmentation due to UFO, select a new fragment id.
> >>>>>>>>>> When we store the fragment id into skb_shinfo, set the bit
> >>>>>>>>>> in the skb so we can re-use the selected id.
> >>>>>>>>>> This preserves the behavior of UFO packets generated on the
> >>>>>>>>>> host and solves the issue of id generation for packet sockets
> >>>>>>>>>> and tap/macvtap devices.
> >>>>>>>>>>
> >>>>>>>>>> This patch moves ipv6_select_ident() back in to the header file.
> >>>>>>>>>> It also provides the helper function that sets skb_shinfo() frag
> >>>>>>>>>> id and sets the bit.
> >>>>>>>>>>
> >>>>>>>>>> It also makes sure that we select the fragment id when doing
> >>>>>>>>>> just gso validation, since it's possible for the packet to
> >>>>>>>>>> come from an untrusted source (VM) and be forwarded through
> >>>>>>>>>> a UFO enabled device which will expect the fragment id.
> >>>>>>>>>>
> >>>>>>>>>> CC: Eric Dumazet <edumazet@google.com>
> >>>>>>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>> include/linux/skbuff.h | 3 ++-
> >>>>>>>>>> include/net/ipv6.h | 2 ++
> >>>>>>>>>> net/ipv6/ip6_output.c | 4 ++--
> >>>>>>>>>> net/ipv6/output_core.c | 9 ++++++++-
> >>>>>>>>>> net/ipv6/udp_offload.c | 10 +++++++++-
> >>>>>>>>>> 5 files changed, 23 insertions(+), 5 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >>>>>>>>>> index 85ab7d7..3ad5203 100644
> >>>>>>>>>> --- a/include/linux/skbuff.h
> >>>>>>>>>> +++ b/include/linux/skbuff.h
> >>>>>>>>>> @@ -605,7 +605,8 @@ struct sk_buff {
> >>>>>>>>>> __u8 ipvs_property:1;
> >>>>>>>>>> __u8 inner_protocol_type:1;
> >>>>>>>>>> __u8 remcsum_offload:1;
> >>>>>>>>>> - /* 3 or 5 bit hole */
> >>>>>>>>>> + __u8 ufo_fragid_set:1;
> >>>>>>>>> [...]
> >>>>>>>>>
> >>>>>>>>> Doesn't the flag belong in struct skb_shared_info, rather than struct
> >>>>>>>>> sk_buff? Otherwise this looks fine.
> >>>>>>>>>
> >>>>>>>>> Ben.
> >>>>>>>>
> >>>>>>>> Hmm we seem to be out of tx flags.
> >>>>>>>> Maybe ip6_frag_id == 0 should mean "not set".
> >>>>>>>
> >>>>>>> Maybe that is the best idea. Definitely the ufo_fragid_set bit should
> >>>>>>> move into the skb_shared_info area.
> >>>>>>
> >>>>>> That's what I originally wanted to do, but had to move and grow txflags thus
> >>>>>> skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
> >>>>>>
> >>>>>> I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
> >>>>>> from the protocol perspective and could actually be generated by the id generator
> >>>>>> functions. This may cause us to call the id generation multiple times.
> >>>>>
> >>>>> Are there plans in the long run to let virtio_net transmit auxiliary
> >>>>> data to the other end so we can clean all of this this up one day?
> >>>>>
> >>>>> I don't like the whole situation: looking into the virtio_net headers
> >>>>> just adding a field for ipv6 fragmentation ids to those small structs
> >>>>> seems bloated, not doing it feels incorrect. :/
> >>>>>
> >>>>> Thoughts?
> >>>>>
> >>>>> Bye,
> >>>>> Hannes
> >>>>
> >>>> I'm not sure - what will be achieved by generating the IDs guest side as
> >>>> opposed to host side? It's certainly harder to get hold of entropy
> >>>> guest-side.
> >>>
> >>> It is not only about entropy but about uniqueness. Also fragmentation
> >>> ids should not be discoverable,
> >>
> >> I belive "predictable" is the language used by the IETF draft.
> >>
> >>> so there are several aspects:
> >>>
> >>> I see fragmentation id generation still as security critical:
> >>> When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> >>> identifiers less predictable") I could patch my kernels and use the
> >>> patch regardless of the machine being virtualized or not. It was not
> >>> dependent on the hypervisor.
> >>
> >> And now it's even easier - just patch the hypervisor, and all VMs
> >> automatically benefit.
> >
> > Sometimes the hypervisor is not under my control. You would need to
> > patch both kernels in your case - non gso frames would still get the
> > fragmentation id generated in the host kernel.
>
> Why would non-gso frames need a frag id? We are talking only UDP IPv6
> here, so there is no frag id generation if the packet does't need to
> be fragmented.
E.g. raw sockets still can generate fragments locally. It is also a
valid setup to have multiple interfaces in one machine, one that is UFO
enabled and one that isn't. In that case, fragmentation id generation
happens on different hosts which I want to avoid.
I haven't looked closely but mismatch of MTUs on interfaces seems like
it could lead to unwanted fragmentation, e.g. see is_skb_forwardable
which is mostly always true for gso frames, so we never stop them on
bridges etc.
> >>> I think that is the same reasoning why we
> >>> don't support TOE.
> >>> If we use one generator in the hypervisor in an openstack alike setting,
> >>> the host deals with quite a lot of overlay networks. A lot of default
> >>> configurations use the same addresses internally, so on the hypervisor
> >>> the frag id generators would interfere by design.
> >>> I could come up with an attack scenario for DNS servers (again :) ):
> >>>
> >>> You are sitting next to a DNS server on the same hypervisor and can send
> >>> packets without source validation (because that is handled later on in
> >>> case of openvswitch when the packet is put into the corresponding
> >>> overlay network). You emit a gso packet with the same source and
> >>> destination addresses as the DNS server would do and would get an
> >>> fragmentation id which is linearly (+ time delta) incremented depending
> >>> on the source and destination address. With such a leak you could start
> >>> trying attack and spoof DNS responses (fragmentation attacks etc.).
> >>> See also details on such kind of attacks in the description of commit
> >>> 04ca6973f7c1a0d.
> >>>
> >>> AFAIK IETF tried with IPv6 to push fragmentation id generation to the
> >>> end hosts, that's also the reason for the introduction of atomic
> >>> fragments (which are now being rolled back ;) ).
> >>>
> >>> Still it is better to generate a frag id on the hypervisor than just
> >>> sending a 0, so I am ok with this change, albeit not happy.
> >>>
> >>> Thanks,
> >>> Hannes
> >>>
> >>
> >> OK so to summarize, identifiers are only re-randomized once per jiffy,
> >> so you worry that within this window, an external observer can discover
> >> past fragment ID values and so predict the future ones.
> >> All that's required is that two paths go through the same box performing
> >> fragmentation.
> >>
> >> Is that a fair summary?
> >>
> >> If yes, we can make this a bit harder by mixing in some
> >> data per input and/or output devices.
> >>
> >> For example, just to give you the idea:
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 683d493..4faa7ef 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> >> trace_netif_receive_skb(skb);
> >>
> >> orig_dev = skb->dev;
> >> + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
> >>
> >> skb_reset_network_header(skb);
> >> if (!skb_transport_header_was_set(skb))
> >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> >> index ce69a12..819a821 100644
> >> --- a/net/ipv6/ip6_output.c
> >> +++ b/net/ipv6/ip6_output.c
> >> @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> >> sizeof(struct frag_hdr)) & ~7;
> >> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> >> ipv6_select_ident(&fhdr, rt);
> >> - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> >> + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> >> + fhdr.identification);
> >>
> >> append:
> >> return skb_append_datato_frags(sk, skb, getfrag, from,
> >>
> >
> > I thought about mixing in the incoming interface identifier into the
> > frag id generation, but that could hurt us badly as soon as a VM has
> > more than one interface to the outside world and uses e.g. ECMP. We need
> > to make sure that those frag ids are unique and the kernel needs to be
> > better than just using a random number generator.
> >
>
> So the goal behind this series of patches is to restore VM functionality to
> pre-916e4cf46d0204 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data").
I understand (the patch fixed a NULL ptr deref btw.).
As I said, I don't want to stop this series (hopefully the flag can be
moved into skb_shared_info etc.), would look after that IMHO
(skb flags/IPCB and skb_shared_info have different semantics on
__skb_clone).
I think it is very much worth to try to move the fragmentation id
generation back to the end host and only use this as a fallback.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Hannes Frederic Sowa @ 2015-01-28 14:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Vladislav Yasevich, virtualization, edumazet,
Ben Hutchings
In-Reply-To: <20150128134335.GA31959@redhat.com>
On Mi, 2015-01-28 at 15:43 +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote:
> > Hi,
> >
> > On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote:
> > > On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote:
> > > > Hello,
> > > >
> > > > On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote:
> > > > > On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote:
> > > > > > On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote:
> > > > > > > On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote:
> > > > > > > > On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote:
> > > > > > > >> On Tue, Jan 27, 2015 at 02:47:54AM +0000, Ben Hutchings wrote:
> > > > > > > >>> On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote:
> > > > > > > >>>> If the IPv6 fragment id has not been set and we perform
> > > > > > > >>>> fragmentation due to UFO, select a new fragment id.
> > > > > > > >>>> When we store the fragment id into skb_shinfo, set the bit
> > > > > > > >>>> in the skb so we can re-use the selected id.
> > > > > > > >>>> This preserves the behavior of UFO packets generated on the
> > > > > > > >>>> host and solves the issue of id generation for packet sockets
> > > > > > > >>>> and tap/macvtap devices.
> > > > > > > >>>>
> > > > > > > >>>> This patch moves ipv6_select_ident() back in to the header file.
> > > > > > > >>>> It also provides the helper function that sets skb_shinfo() fragd have to patch both kernels *in your case*.
If it's all done by host, then it's in a single place, on host.
> > > > > > > >>>> id and sets the bit.
> > > > > > > >>>>
> > > > > > > >>>> It also makes sure that we select the fragment id when doing
> > > > > > > >>>> just gso validation, since it's possible for the packet to
> > > > > > > >>>> come from an untrusted source (VM) and be forwarded through
> > > > > > > >>>> a UFO enabled device which will expect the fragment id.
> > > > > > > >>>>
> > > > > > > >>>> CC: Eric Dumazet <edumazet@google.com>
> > > > > > > >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> > > > > > > >>>> ---
> > > > > > > >>>> include/linux/skbuff.h | 3 ++-
> > > > > > > >>>> include/net/ipv6.h | 2 ++
> > > > > > > >>>> net/ipv6/ip6_output.c | 4 ++--
> > > > > > > >>>> net/ipv6/output_core.c | 9 ++++++++-
> > > > > > > >>>> net/ipv6/udp_offload.c | 10 +++++++++-
> > > > > > > >>>> 5 files changed, 23 insertions(+), 5 deletions(-)
> > > > > > > >>>>
> > > > > > > >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > > > > > >>>> index 85ab7d7..3ad5203 100644
> > > > > > > >>>> --- a/include/linux/skbuff.h
> > > > > > > >>>> +++ b/include/linux/skbuff.h
> > > > > > > >>>> @@ -605,7 +605,8 @@ struct sk_buff {
> > > > > > > >>>> __u8 ipvs_property:1;
> > > > > > > >>>> __u8 inner_protocol_type:1;
> > > > > > > >>>> __u8 remcsum_offload:1;
> > > > > > > >>>> - /* 3 or 5 bit hole */
> > > > > > > >>>> + __u8 ufo_fragid_set:1;
> > > > > > > >>> [...]
> > > > > > > >>>
> > > > > > > >>> Doesn't the flag belong in struct skb_shared_info, rather than struct
> > > > > > > >>> sk_buff? Otherwise this looks fine.
> > > > > > > >>>
> > > > > > > >>> Ben.
> > > > > > > >>
> > > > > > > >> Hmm we seem to be out of tx flags.
> > > > > > > >> Maybe ip6_frag_id == 0 should mean "not set".
> > > > > > > >
> > > > > > > > Maybe that is the best idea. Definitely the ufo_fragid_set bit should
> > > > > > > > move into the skb_shared_info area.
> > > > > > >
> > > > > > > That's what I originally wanted to do, but had to move and grow txflags thus
> > > > > > > skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag.
> > > > > > >
> > > > > > > I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid
> > > > > > > from the protocol perspective and could actually be generated by the id generator
> > > > > > > functions. This may cause us to call the id generation multiple times.
> > > > > >
> > > > > > Are there plans in the long run to let virtio_net transmit auxiliary
> > > > > > data to the other end so we can clean all of this this up one day?
> > > > > >
> > > > > > I don't like the whole situation: looking into the virtio_net headers
> > > > > > just adding a field for ipv6 fragmentation ids to those small structs
> > > > > > seems bloated, not doing it feels incorrect. :/
> > > > > >
> > > > > > Thoughts?
> > > > > >
> > > > > > Bye,
> > > > > > Hannes
> > > > >
> > > > > I'm not sure - what will be achieved by generating the IDs guest side as
> > > > > opposed to host side? It's certainly harder to get hold of entropy
> > > > > guest-side.
> > > >
> > > > It is not only about entropy but about uniqueness. Also fragmentation
> > > > ids should not be discoverable,
> > >
> > > I belive "predictable" is the language used by the IETF draft.
> > >
> > > > so there are several aspects:
> > > >
> > > > I see fragmentation id generation still as security critical:
> > > > When Eric patched the frag id generator in 04ca6973f7c1a0d ("ip: make IP
> > > > identifiers less predictable") I could patch my kernels and use the
> > > > patch regardless of the machine being virtualized or not. It was not
> > > > dependent on the hypervisor.
> > >
> > > And now it's even easier - just patch the hypervisor, and all VMs
> > > automatically benefit.
> >
> > Sometimes the hypervisor is not under my control. You would need to
> > patch both kernels in your case - non gso frames would still get the
> > fragmentation id generated in the host kernel.
>
> Confused. You would have to patch both kernels *in your case*.
> If it's all done by host, then it's in a single place, on host.
host is the hypervisor?
Anyway, we would have to patch both kernels now anyway. :)
We still have a working ipv6_fragment routine in the virtualized kernel
which can embed fragmentation extension headers in frames, thus
generating the fragmentation id in the virtualized kernel.
> > > > I think that is the same reasoning why we
> > > > don't support TOE.
> > > > If we use one generator in the hypervisor in an openstack alike setting,
> > > > the host deals with quite a lot of overlay networks. A lot of default
> > > > configurations use the same addresses internally, so on the hypervisor
> > > > the frag id generators would interfere by design.
> > > > I could come up with an attack scenario for DNS servers (again :) ):
> > > >
> > > > You are sitting next to a DNS server on the same hypervisor and can send
> > > > packets without source validation (because that is handled later on in
> > > > case of openvswitch when the packet is put into the corresponding
> > > > overlay network). You emit a gso packet with the same source and
> > > > destination addresses as the DNS server would do and would get an
> > > > fragmentation id which is linearly (+ time delta) incremented depending
> > > > on the source and destination address. With such a leak you could start
> > > > trying attack and spoof DNS responses (fragmentation attacks etc.).
> > > > See also details on such kind of attacks in the description of commit
> > > > 04ca6973f7c1a0d.
> > > >
> > > > AFAIK IETF tried with IPv6 to push fragmentation id generation to the
> > > > end hosts, that's also the reason for the introduction of atomic
> > > > fragments (which are now being rolled back ;) ).
> > > >
> > > > Still it is better to generate a frag id on the hypervisor than just
> > > > sending a 0, so I am ok with this change, albeit not happy.
> > > >
> > > > Thanks,
> > > > Hannes
> > > >
> > >
> > > OK so to summarize, identifiers are only re-randomized once per jiffy,
> > > so you worry that within this window, an external observer can discover
> > > past fragment ID values and so predict the future ones.
> > > All that's required is that two paths go through the same box performing
> > > fragmentation.
> > >
> > > Is that a fair summary?
> > >
> > > If yes, we can make this a bit harder by mixing in some
> > > data per input and/or output devices.
> > >
> > > For example, just to give you the idea:
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 683d493..4faa7ef 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -3625,6 +3625,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> > > trace_netif_receive_skb(skb);
> > >
> > > orig_dev = skb->dev;
> > > + skb_shinfo(skb)->ip6_frag_id = skb->dev->ifindex;
> > >
> > > skb_reset_network_header(skb);
> > > if (!skb_transport_header_was_set(skb))
> > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > > index ce69a12..819a821 100644
> > > --- a/net/ipv6/ip6_output.c
> > > +++ b/net/ipv6/ip6_output.c
> > > @@ -1092,7 +1092,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> > > sizeof(struct frag_hdr)) & ~7;
> > > skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > > ipv6_select_ident(&fhdr, rt);
> > > - skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> > > + skb_shinfo(skb)->ip6_frag_id = jhash_1word(skb_shinfo(skb)->ip6_frag_id,
> > > + fhdr.identification);
> > >
> > > append:
> > > return skb_append_datato_frags(sk, skb, getfrag, from,
> > >
> >
> > I thought about mixing in the incoming interface identifier into the
> > frag id generation, but that could hurt us badly as soon as a VM has
> > more than one interface to the outside world and uses e.g. ECMP.
>
> I don't understand. Fragmentation is done after routing,
> isn't it? So all fragments always go out on the same device.
It is the other way around:
(Source, Dest) is the key to lookup the next fragmentation id. We send
two fragments and use (Source, Dest, ifindex) as key, then the first
packet leaves the host on ifindex 1 with fragid x (because it was a big
gso packet it got segments), second packet leaves host on ifindex 2 with
same Source and Dest and results in fragid y. Ideally both those packets
should use the same bucket to improve uniqueness. Without ifindex we
would have y = x+<number segments>, otherwise it would be (I guess)
random.
> > We need
> > to make sure that those frag ids are unique and the kernel needs to be
> > better than just using a random number generator.
> >
> > Bye,
> > Hannes
>
> 32 bit numbers can't be unique. They just shouldn't be discoverable
> by an off-path observer.
No, they can't. But they should be reasonable unique and, as you said,
not discoverable.
Bye,
Hannes
^ permalink raw reply
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