[PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Thomas Graf]

The memset() on the packet data is expensive and severely limiting
the pps throughput for large frame sizes.

Considering that pktgen requires root privileges to run it is safe
to introduce an option to optionally avoid the memset() and leave
the packet data uninitialized.

VM test results, 2 VCPU, 2 threads, pkt_size=9K:

    12.44%  -11.15%  [kernel.kallsyms]   [k] memset
     4.84%   +1.58%  [kernel.kallsyms]   [k] get_page_from_freelist

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 Documentation/networking/pktgen.txt |  3 +++
 net/core/pktgen.c                   | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt
index 0e30c78..349b9d2 100644
--- a/Documentation/networking/pktgen.txt
+++ b/Documentation/networking/pktgen.txt
@@ -114,6 +114,8 @@ Examples:
                               UDPCSUM,
                               IPSEC # IPsec encapsulation (needs CONFIG_XFRM)
                               NODE_ALLOC # node specific memory allocation
+			      NOINIT # leave packet data uninitialized
+			             # (BEWARE! May expose kernel memory!)
 
  pgset spi SPI_VALUE     Set specific SA used to transform packet.
 
@@ -254,6 +256,7 @@ flag
   UDPCSUM
   IPSEC
   NODE_ALLOC
+  NOINIT
 
 dst_min
 dst_max
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 0304f98..8278df8 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -201,6 +201,7 @@
 #define F_QUEUE_MAP_CPU (1<<14)	/* queue map mirrors smp_processor_id() */
 #define F_NODE          (1<<15)	/* Node memory alloc*/
 #define F_UDPCSUM       (1<<16)	/* Include UDP checksum */
+#define F_NOINIT        (1<<17)	/* Keep packet data uninitialized */
 
 /* Thread control flag bits */
 #define T_STOP        (1<<0)	/* Stop run */
@@ -675,6 +676,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
 	if (pkt_dev->flags & F_NODE)
 		seq_printf(seq, "NODE_ALLOC  ");
 
+	if (pkt_dev->flags & F_NOINIT)
+		seq_puts(seq, "NOINIT  ");
+
 	seq_puts(seq, "\n");
 
 	/* not really stopped, more like last-running-at */
@@ -1242,6 +1246,12 @@ static ssize_t pktgen_if_write(struct file *file,
 		else if (strcmp(f, "!UDPCSUM") == 0)
 			pkt_dev->flags &= ~F_UDPCSUM;
 
+		else if (strcmp(f, "NOINIT") == 0)
+			pkt_dev->flags |= F_NOINIT;
+
+		else if (strcmp(f, "!NOINIT") == 0)
+			pkt_dev->flags &= ~F_NOINIT;
+
 		else {
 			sprintf(pg_result,
 				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
@@ -2633,7 +2643,8 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 	datalen -= sizeof(*pgh);
 
 	if (pkt_dev->nfrags <= 0) {
-		memset(skb_put(skb, datalen), 0, datalen);
+		if (!(pkt_dev->flags & F_NOINIT))
+			memset(skb_put(skb, datalen), 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;
 		int i, len;
@@ -2644,7 +2655,8 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 			frags = MAX_SKB_FRAGS;
 		len = datalen - frags * PAGE_SIZE;
 		if (len > 0) {
-			memset(skb_put(skb, len), 0, len);
+			if (!(pkt_dev->flags & F_NOINIT))
+				memset(skb_put(skb, len), 0, len);
 			datalen = frags * PAGE_SIZE;
 		}
 
-- 
1.8.3.1

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Eric Dumazet]

On Fri, 2014-05-16 at 12:34 +0200, Thomas Graf wrote:
> The memset() on the packet data is expensive and severely limiting
> the pps throughput for large frame sizes.
> 
> Considering that pktgen requires root privileges to run it is safe
> to introduce an option to optionally avoid the memset() and leave
> the packet data uninitialized.
> 
> VM test results, 2 VCPU, 2 threads, pkt_size=9K:
> 
>     12.44%  -11.15%  [kernel.kallsyms]   [k] memset
>      4.84%   +1.58%  [kernel.kallsyms]   [k] get_page_from_freelist
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  Documentation/networking/pktgen.txt |  3 +++
>  net/core/pktgen.c                   | 16 ++++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)

We do not want to leak content of memory, even if 'root' is willing to.

Sorry, you need to do something else if you really care.

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Ben Greear]

On 05/16/2014 06:16 AM, Eric Dumazet wrote:
> On Fri, 2014-05-16 at 12:34 +0200, Thomas Graf wrote:
>> The memset() on the packet data is expensive and severely limiting
>> the pps throughput for large frame sizes.
>>
>> Considering that pktgen requires root privileges to run it is safe
>> to introduce an option to optionally avoid the memset() and leave
>> the packet data uninitialized.
>>
>> VM test results, 2 VCPU, 2 threads, pkt_size=9K:
>>
>>      12.44%  -11.15%  [kernel.kallsyms]   [k] memset
>>       4.84%   +1.58%  [kernel.kallsyms]   [k] get_page_from_freelist
>>
>> Signed-off-by: Thomas Graf <tgraf@suug.ch>
>> ---
>>   Documentation/networking/pktgen.txt |  3 +++
>>   net/core/pktgen.c                   | 16 ++++++++++++++--
>>   2 files changed, 17 insertions(+), 2 deletions(-)
>
> We do not want to leak content of memory, even if 'root' is willing to.
>
> Sorry, you need to do something else if you really care.

Is there currently no other way for root to grab memory contents and
send them over the network?

I mean, it could load it's own module and do bad things if nothing else?

Thanks,
Ben

>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Eric Dumazet]

On Fri, 2014-05-16 at 06:28 -0700, Ben Greear wrote:
> 
> On 05/16/2014 06:16 AM, Eric Dumazet wrote:
> > On Fri, 2014-05-16 at 12:34 +0200, Thomas Graf wrote:
> >> The memset() on the packet data is expensive and severely limiting
> >> the pps throughput for large frame sizes.
> >>
> >> Considering that pktgen requires root privileges to run it is safe
> >> to introduce an option to optionally avoid the memset() and leave
> >> the packet data uninitialized.
> >>
> >> VM test results, 2 VCPU, 2 threads, pkt_size=9K:
> >>
> >>      12.44%  -11.15%  [kernel.kallsyms]   [k] memset
> >>       4.84%   +1.58%  [kernel.kallsyms]   [k] get_page_from_freelist
> >>
> >> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> >> ---
> >>   Documentation/networking/pktgen.txt |  3 +++
> >>   net/core/pktgen.c                   | 16 ++++++++++++++--
> >>   2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > We do not want to leak content of memory, even if 'root' is willing to.
> >
> > Sorry, you need to do something else if you really care.
> 
> Is there currently no other way for root to grab memory contents and
> send them over the network?
> 
> I mean, it could load it's own module and do bad things if nothing else?

pktgen has already a way to send fragments which are not memset for
every packet. We share a 'zero' page for that.

So only headers need to be really built for each packet.

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Thomas Graf]

On 05/16/14 at 07:23am, Eric Dumazet wrote:
> pktgen has already a way to send fragments which are not memset for
> every packet. We share a 'zero' page for that.
> 
> So only headers need to be really built for each packet.

Right, F_NOINIT is exclusively for cases where a linear buffer is
desired. I don't fully understand the argument of exposing kernel
memory as root considering the presence of /dev/mem though.

That said, I understand that this is a special case and that the
objective is reachable with non linear skbs already so I'm fine
with ignoring this patch and have it linger outside of the tree for
whoever has use for it.

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Eric Dumazet]

On Fri, 2014-05-16 at 15:50 +0100, Thomas Graf wrote:
> On 05/16/14 at 07:23am, Eric Dumazet wrote:
> > pktgen has already a way to send fragments which are not memset for
> > every packet. We share a 'zero' page for that.
> > 
> > So only headers need to be really built for each packet.
> 
> Right, F_NOINIT is exclusively for cases where a linear buffer is
> desired. I don't fully understand the argument of exposing kernel
> memory as root considering the presence of /dev/mem though.
> 
> That said, I understand that this is a special case and that the
> objective is reachable with non linear skbs already so I'm fine
> with ignoring this patch and have it linger outside of the tree for
> whoever has use for it.

Note that if you really want to save cpu cycles, you might simply avoid
kfree_skb()/alloc_skb() pair, and reuse the previous skb.

Yes, pktgen can vary the skb->len, but this doesn't mean we can
allocate/reserve a big skb->head. Then the cost of initial memset() for
this skb is peanuts.

Synch with Jesper because he is trying to save the atomic on skb->users.

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Thomas Graf]

On 05/16/14 at 08:08am, Eric Dumazet wrote:
> On Fri, 2014-05-16 at 15:50 +0100, Thomas Graf wrote:
> > On 05/16/14 at 07:23am, Eric Dumazet wrote:
> > > pktgen has already a way to send fragments which are not memset for
> > > every packet. We share a 'zero' page for that.
> > > 
> > > So only headers need to be really built for each packet.
> > 
> > Right, F_NOINIT is exclusively for cases where a linear buffer is
> > desired. I don't fully understand the argument of exposing kernel
> > memory as root considering the presence of /dev/mem though.
> > 
> > That said, I understand that this is a special case and that the
> > objective is reachable with non linear skbs already so I'm fine
> > with ignoring this patch and have it linger outside of the tree for
> > whoever has use for it.
> 
> Note that if you really want to save cpu cycles, you might simply avoid
> kfree_skb()/alloc_skb() pair, and reuse the previous skb.
> 
> Yes, pktgen can vary the skb->len, but this doesn't mean we can
> allocate/reserve a big skb->head. Then the cost of initial memset() for
> this skb is peanuts.
> 
> Synch with Jesper because he is trying to save the atomic on skb->users.

I'm aware of the work and I agree. That is perfectly fine if the
produced skbs do not get enqueued and modified. Otherwise the clone
and COW will undo the gain again. I need pktgen to produce large,
linear skbs with a unique flow for each individual packet. Hence this
optimization ;-)

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Eric Dumazet]

On Fri, 2014-05-16 at 16:20 +0100, Thomas Graf wrote:

> I'm aware of the work and I agree. That is perfectly fine if the
> produced skbs do not get enqueued and modified. Otherwise the clone
> and COW will undo the gain again. I need pktgen to produce large,
> linear skbs with a unique flow for each individual packet. Hence this
> optimization ;-)

pktgen is a tool for us kernel net developers.

I doubt more than 20 people on this planet used it in the last 12
months, expecting crazy numbers out of one core, while a simple user
space program can outperform pktgen these days.

So if you need a very specific hack, you can do it on your local tree ;)

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Thomas Graf]

On 05/16/14 at 08:26am, Eric Dumazet wrote:
> On Fri, 2014-05-16 at 16:20 +0100, Thomas Graf wrote:
> 
> > I'm aware of the work and I agree. That is perfectly fine if the
> > produced skbs do not get enqueued and modified. Otherwise the clone
> > and COW will undo the gain again. I need pktgen to produce large,
> > linear skbs with a unique flow for each individual packet. Hence this
> > optimization ;-)
> 
> pktgen is a tool for us kernel net developers.
> 
> I doubt more than 20 people on this planet used it in the last 12
> months, expecting crazy numbers out of one core, while a simple user
> space program can outperform pktgen these days.
> 
> So if you need a very specific hack, you can do it on your local tree ;)

Which I already stated I'd be fine with ;)

But for the sake of the argument... Since you mention that pktgen is
clearly a tool for us kernel net developers. That would (hopefully)
imply that everybody using it knows what they are doing. Just like
somebody running kgdb, dumping kernel memory with kdump, or running
perf. I don't see why the choice of deliberately exposing kernel memory
in either of these cases is an issue.

Re: [PATCH net-next] pktgen: Add NOINIT option to leave packet data uninitialized

[Eric Dumazet]

On Fri, 2014-05-16 at 16:39 +0100, Thomas Graf wrote:

>  I don't see why the choice of deliberately exposing kernel memory
> in either of these cases is an issue.

This has been discussed a long time ago, when I had such claims, as root
user "should be able to get dirty pages". If I remember, this was a new
mmap() flag or something like that.

(Ie not requiring clear new pages)

This was of course rejected for upstream linux, for _very_ good reasons.

With the advent of network namespaces, you could have the right to use
pktgen on your ethernet device, without allowing you to read arbitrary
kernel memory.

The day its done, we would have to revert your change, and it is very
possible nobody catch this dependency.

Really this is simply about basic security concerns with an incredibly
complex code base.