Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next 3/3] net: allow to leave the buffer fragmented in skb_cow_data()
From: Eric Dumazet @ 2013-11-05 14:33 UTC (permalink / raw)
  To: Mathias Krause
  Cc: David S. Miller, Steffen Klassert, Herbert Xu, Dmitry Tarnyagin,
	netdev
In-Reply-To: <14f30e8f5f8405c1ca73b6d3a554441c1736142d.1381923854.git.mathias.krause@secunet.com>

On Tue, 2013-11-05 at 14:54 +0100, Mathias Krause wrote:
> Do not linearize the buffer per se but only if we're expected to expand
> the tail. All callers can handle fragmented buffers and even expect
> them!
> 
> Not linearizing the buffer leads to a small performance improvement for
> the IPsec receive path in case the network driver passed us a fragmented
> buffer.
> 
> With this patch applied I was able to increase the throughput of an
> IPsec gateway from 7.12 Gbit/s to 7.28 Gbit/s.

What is the driver you used ?

^ permalink raw reply

* Re: [PATCH net] vti: fix spd lookup: match plaintext pkt, not ipsec pkt
From: Christophe Gouault @ 2013-11-05 14:31 UTC (permalink / raw)
  To: Sergei Shtylyov, Steffen Klassert, David S. Miller
  Cc: Herbert Xu, Saurabh Mohan, netdev
In-Reply-To: <5278ED2C.8070604@cogentembedded.com>

Hello Sergei,

On 11/05/2013 02:05 PM, Sergei Shtylyov wrote:
>> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
>> index 6e87f85..a7e03c0 100644
>> --- a/net/ipv4/ip_vti.c
>> +++ b/net/ipv4/ip_vti.c
> [...]
>> @@ -133,7 +134,12 @@ static int vti_rcv(struct sk_buff *skb)
>>            * only match policies with this mark.
>>            */
>>           skb->mark = be32_to_cpu(tunnel->parms.o_key);
>> +        /* the packet is decrypted, but not yet decapsulated.
>> +         * Temporarily make network_header point to the inner header
>> +         * for policy check */
>
>    Multi-line comment style in the networking code is:
>
> /* bla
>  * bla
>  */
Sorry for that, I was not careful enough. I'll fix it right now.

Thanks and Best Regards,
Christophe

^ permalink raw reply

* [PATCH net-next 4/4] ipv6: protect flow label renew against GC
From: Florent Fourcot @ 2013-11-05 14:28 UTC (permalink / raw)
  To: netdev; +Cc: Florent Fourcot
In-Reply-To: <1383661738-6083-1-git-send-email-florent.fourcot@enst-bretagne.fr>

Take ip6_fl_lock before to read and update a
label. It prevents race condition if GC is
running.

Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
---
 net/ipv6/ip6_flowlabel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index 41ced9c..1d2fc48 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -540,11 +540,13 @@ int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen)
 		return -ESRCH;
 
 	case IPV6_FL_A_RENEW:
+		spin_lock_bh(&ip6_fl_lock);
 		rcu_read_lock_bh();
 		for_each_sk_fl_rcu(np, sfl) {
 			if (sfl->fl->label == freq.flr_label) {
 				err = fl6_renew(sfl->fl, freq.flr_linger, freq.flr_expires);
 				rcu_read_unlock_bh();
+				spin_unlock_bh(&ip6_fl_lock);
 				return err;
 			}
 		}
@@ -555,10 +557,12 @@ int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen)
 			fl = fl_lookup(net, freq.flr_label);
 			if (fl) {
 				err = fl6_renew(fl, freq.flr_linger, freq.flr_expires);
+				spin_unlock_bh(&ip6_fl_lock);
 				fl_release(fl);
 				return err;
 			}
 		}
+		spin_unlock_bh(&ip6_fl_lock);
 		return -ESRCH;
 
 	case IPV6_FL_A_GET:
-- 
1.8.4.rc3

^ permalink raw reply related

* [PATCH net-next v2 2/4] ipv6: enable IPV6_FLOWLABEL_MGR for getsockopt
From: Florent Fourcot @ 2013-11-05 14:28 UTC (permalink / raw)
  To: netdev; +Cc: Florent Fourcot
In-Reply-To: <1383661738-6083-1-git-send-email-florent.fourcot@enst-bretagne.fr>

It is already possible to set/put/renew a label
with IPV6_FLOWLABEL_MGR and setsockopt. This patch
add the possibility to get information about this
label (current value, time before expiration, etc).

It helps application to take decision for a renew
or a release of the label.

v2:
 * Add spin_lock to prevent race condition
 * return -ENOENT if no result found
 * check if flr_action is GET

Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
---
 include/net/ipv6.h       |  1 +
 net/ipv6/ip6_flowlabel.c | 27 +++++++++++++++++++++++++++
 net/ipv6/ipv6_sockglue.c | 28 ++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index dd96638..2a5f668 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -250,6 +250,7 @@ struct ipv6_txoptions *fl6_merge_options(struct ipv6_txoptions *opt_space,
 					 struct ipv6_txoptions *fopt);
 void fl6_free_socklist(struct sock *sk);
 int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen);
+int ipv6_flowlabel_opt_get(struct sock *sk, struct in6_flowlabel_req *freq);
 int ip6_flowlabel_init(void);
 void ip6_flowlabel_cleanup(void);
 
diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index 819578e..76e62a1 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -475,6 +475,33 @@ static inline void fl_link(struct ipv6_pinfo *np, struct ipv6_fl_socklist *sfl,
 	spin_unlock_bh(&ip6_sk_fl_lock);
 }
 
+int ipv6_flowlabel_opt_get(struct sock *sk, struct in6_flowlabel_req *freq)
+{
+	struct ipv6_pinfo *np = inet6_sk(sk);
+	struct ipv6_fl_socklist *sfl;
+
+	spin_lock_bh(&ip6_fl_lock);
+	rcu_read_lock_bh();
+
+	for_each_sk_fl_rcu(np, sfl) {
+		if (sfl->fl->label == (np->flow_label & IPV6_FLOWLABEL_MASK)) {
+			freq->flr_label = sfl->fl->label;
+			freq->flr_dst = sfl->fl->dst;
+			freq->flr_share = sfl->fl->share;
+			freq->flr_expires = (sfl->fl->expires - jiffies) / HZ;
+			freq->flr_linger = sfl->fl->linger / HZ;
+
+			rcu_read_unlock_bh();
+			spin_unlock_bh(&ip6_fl_lock);
+			return 0;
+		}
+	}
+	rcu_read_unlock_bh();
+	spin_unlock_bh(&ip6_fl_lock);
+
+	return -ENOENT;
+}
+
 int ipv6_flowlabel_opt(struct sock *sk, char __user *optval, int optlen)
 {
 	int uninitialized_var(err);
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4919a8e..1c6ce31 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -1212,6 +1212,34 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
 		val = np->sndflow;
 		break;
 
+	case IPV6_FLOWLABEL_MGR:
+	{
+		struct in6_flowlabel_req freq;
+
+		if (len < sizeof(freq))
+			return -EINVAL;
+
+		if (copy_from_user(&freq, optval, sizeof(freq)))
+			return -EFAULT;
+
+		if (freq.flr_action != IPV6_FL_A_GET)
+			return -EINVAL;
+
+		len = sizeof(freq);
+		memset(&freq, 0, sizeof(freq));
+
+		val = ipv6_flowlabel_opt_get(sk, &freq);
+		if (val < 0)
+			return val;
+
+		if (put_user(len, optlen))
+			return -EFAULT;
+		if (copy_to_user(optval, &freq, len))
+			return -EFAULT;
+
+		return 0;
+	}
+
 	case IPV6_ADDR_PREFERENCES:
 		val = 0;
 
-- 
1.8.4.rc3

^ permalink raw reply related

* [PATCH net-next 3/4] ipv6: increase maximum lifetime of flow labels
From: Florent Fourcot @ 2013-11-05 14:28 UTC (permalink / raw)
  To: netdev; +Cc: Florent Fourcot
In-Reply-To: <1383661738-6083-1-git-send-email-florent.fourcot@enst-bretagne.fr>

If the last RFC 6437 does not give any constraints
for lifetime of flow labels, the previous RFC 3697
spoke of a minimum of 120 seconds quarantine
for a flow label.

The maximum linger is currently set to 60 seconds
and does not allow this configuration without
CAP_NET_ADMIN right.

This patch increases the maximum linger to 150
seconds, allowing more flexibility to standard
users.

Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
---
 net/ipv6/ip6_flowlabel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index 76e62a1..41ced9c 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -41,7 +41,7 @@
 #define FL_MIN_LINGER	6	/* Minimal linger. It is set to 6sec specified
 				   in old IPv6 RFC. Well, it was reasonable value.
 				 */
-#define FL_MAX_LINGER	60	/* Maximal linger timeout */
+#define FL_MAX_LINGER	150	/* Maximal linger timeout */
 
 /* FL hash table */
 
-- 
1.8.4.rc3

^ permalink raw reply related

* [PATCH net-next v3 1/4] ipv6: remove old conditions on flow label sharing
From: Florent Fourcot @ 2013-11-05 14:28 UTC (permalink / raw)
  To: netdev; +Cc: Florent Fourcot

The code of flow label in Linux Kernel follows
the rules of RFC 1809 (an informational one) for
conditions on flow label sharing. There rules are
not in the last proposed standard for flow label
(RFC 6437), or in the previous one (RFC 3697).

Since this code does not follow any current or
old standard, we can remove it.

With this removal, the ipv6_opt_cmp and
pv6_hdr_cmp function are now a dead code and it
can be removed too.

Changelog of v2:
 * add justification for the change
 * remove the condition on IPv6 options
 * remove ipv6_opt_cmp
v3:
 * remove ipv6_hdr_cmp

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
---
 net/ipv6/ip6_flowlabel.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
index 46e8843..819578e 100644
--- a/net/ipv6/ip6_flowlabel.c
+++ b/net/ipv6/ip6_flowlabel.c
@@ -465,34 +465,6 @@ static int mem_check(struct sock *sk)
 	return 0;
 }
 
-static bool ipv6_hdr_cmp(struct ipv6_opt_hdr *h1, struct ipv6_opt_hdr *h2)
-{
-	if (h1 == h2)
-		return false;
-	if (h1 == NULL || h2 == NULL)
-		return true;
-	if (h1->hdrlen != h2->hdrlen)
-		return true;
-	return memcmp(h1+1, h2+1, ((h1->hdrlen+1)<<3) - sizeof(*h1));
-}
-
-static bool ipv6_opt_cmp(struct ipv6_txoptions *o1, struct ipv6_txoptions *o2)
-{
-	if (o1 == o2)
-		return false;
-	if (o1 == NULL || o2 == NULL)
-		return true;
-	if (o1->opt_nflen != o2->opt_nflen)
-		return true;
-	if (ipv6_hdr_cmp(o1->hopopt, o2->hopopt))
-		return true;
-	if (ipv6_hdr_cmp(o1->dst0opt, o2->dst0opt))
-		return true;
-	if (ipv6_hdr_cmp((struct ipv6_opt_hdr *)o1->srcrt, (struct ipv6_opt_hdr *)o2->srcrt))
-		return true;
-	return false;
-}
-
 static inline void fl_link(struct ipv6_pinfo *np, struct ipv6_fl_socklist *sfl,
 		struct ip6_flowlabel *fl)
 {
@@ -603,11 +575,6 @@ recheck:
 				     uid_eq(fl1->owner.uid, fl->owner.uid)))
 					goto release;
 
-				err = -EINVAL;
-				if (!ipv6_addr_equal(&fl1->dst, &fl->dst) ||
-				    ipv6_opt_cmp(fl1->opt, fl->opt))
-					goto release;
-
 				err = -ENOMEM;
 				if (sfl1 == NULL)
 					goto release;
-- 
1.8.4.rc3

^ permalink raw reply related

* Re: [PATCH v2 2/2] ixgbe: enable l2 forwarding acceleration for macvlans
From: Neil Horman @ 2013-11-05 14:26 UTC (permalink / raw)
  To: John Fastabend; +Cc: alexander.h.duyck, netdev, andy, davem, jeffrey.t.kirsher
In-Reply-To: <20131104190154.11802.75724.stgit@jf-dev1-dcblab>

On Mon, Nov 04, 2013 at 11:01:55AM -0800, John Fastabend wrote:
> Now that l2 acceleration ops are in place from the prior patch,
> enable ixgbe to take advantage of these operations.  Allow it to
> allocate queues for a macvlan so that when we transmit a frame,
> we can do the switching in hardware inside the ixgbe card, rather
> than in software.
> 
> For now this patch limits the hardware to 8 offloaded macvlan ports.
> A follow on patch will remove this limitation but to simplify
> review/validation of the new macvlan offload ops we leave it at 8
> for now.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: "David S. Miller" <davem@davemloft.net>

A few Nits, since you're going to send a V3 anyway.  Comments inline.

> ---
><snip> 
> +static void ixgbe_configure_vsi(struct ixgbe_adapter *adapter)
> +{
> +	struct net_device *upper;
> +	struct list_head *iter;
> +	int err;
> +
> +	netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) {
> +		if (netif_is_macvlan(upper)) {
> +			struct macvlan_dev *vlan = netdev_priv(upper);
> +			struct ixgbe_fwd_adapter *vadapter = vlan->fwd_priv;
> +
> +			if (vlan->fwd_priv) {
> +				err = ixgbe_fwd_ring_up(upper, vadapter);
> +				if (err)
> +					continue;
> +				ixgbe_macvlan_set_rx_mode(upper,
> +							  vadapter->pool,
> +							  adapter);
Nit: You're using macvlan here, and vsi above.  Might be nice to give everything
a consistent suffix/prefix for clarity.  Perhaps dfwd since thats what the new
ndo_ops uses?

><snip>
  
>  static inline bool ixgbe_is_sfp(struct ixgbe_hw *hw)
> @@ -4317,6 +4521,8 @@ static void ixgbe_setup_gpie(struct ixgbe_adapter *adapter)
>  static void ixgbe_up_complete(struct ixgbe_adapter *adapter)
>  {
>  	struct ixgbe_hw *hw = &adapter->hw;
> +	struct net_device *upper;
> +	struct list_head *iter;
>  	int err;
>  	u32 ctrl_ext;
>  
> @@ -4360,6 +4566,16 @@ static void ixgbe_up_complete(struct ixgbe_adapter *adapter)
>  	/* enable transmits */
>  	netif_tx_start_all_queues(adapter->netdev);
>  
> +	/* enable any upper devices */
> +	netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) {
> +		if (netif_is_macvlan(upper)) {
> +			struct macvlan_dev *vlan = netdev_priv(upper);
> +
> +			if (vlan->fwd_priv)
> +				netif_tx_start_all_queues(upper);
> +		}
> +	}
> +
I don't think it matters much, but do we want to start the upper devs queues
here unilaterally?  Nominally a network interface starts its queues on dev_open.
Would it be better to only start those devices if (vlan->fwd_priv &&
(upper->flags & IFF_UP)?  We would then have to listen for NETDEV_UP events as
well to handle accelerated macvlans comming up after ixgbe does, which is more
work, but I wanted to mention this in case theres some disadvantage to starting
the queue early.


Other than that I think it looks good though.  Nice work!

Regards
Neil

^ permalink raw reply

* Re: [PATCH v2 1/2] net: Add layer 2 hardware acceleration operations for macvlan devices
From: Neil Horman @ 2013-11-05 14:15 UTC (permalink / raw)
  To: John Fastabend; +Cc: alexander.h.duyck, netdev, andy, davem, jeffrey.t.kirsher
In-Reply-To: <20131104190149.11802.34110.stgit@jf-dev1-dcblab>

On Mon, Nov 04, 2013 at 11:01:49AM -0800, John Fastabend wrote:
> Add a operations structure that allows a network interface to export
> the fact that it supports package forwarding in hardware between
> physical interfaces and other mac layer devices assigned to it (such
> as macvlans). This operaions structure can be used by virtual mac
> devices to bypass software switching so that forwarding can be done
> in hardware more efficiently.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: "David S. Miller" <davem@davemloft.net>
For when you send your V3 patch set out:
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

^ permalink raw reply

* Re: [PATCH 02/10] netfilter: nf_log: prepar net namespace support for nf_log
From: Arnaldo Carvalho de Melo @ 2013-11-05 14:14 UTC (permalink / raw)
  To: Gao feng
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	lve-ke3WRBg/xIg, netfilter-devel-u79uwXL29TY76Z2rM5mHXA,
	ebiederm-aS9lmoZGLiVWk0Htik3J/w, pablo-Cap9r6Oaw4JrovVCs/uTlw
In-Reply-To: <1360223390-15589-2-git-send-email-gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Em Thu, Feb 07, 2013 at 03:49:42PM +0800, Gao feng escreveu:
> index 9e31269..b1e5126 100644
> +++ b/net/netfilter/nf_log.c
> @@ -16,7 +16,6 @@
>  #define NF_LOG_PREFIXLEN		128
>  #define NFLOGGER_NAME_LEN		64
>  
> -static const struct nf_logger __rcu *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;

This was initialized to zeros, but then got moved to struct netns_nf,
I'm trying to figure it out if it is being zeroed somewhere, ideas?

- Arnaldo

^ permalink raw reply

* [PATCH net] qeth: avoid buffer overflow in snmp ioctl
From: Frank Blaschka @ 2013-11-05 14:14 UTC (permalink / raw)
  To: davem; +Cc: netdev, stable

From: Ursula Braun <ursula.braun@de.ibm.com>

Check user-defined length in snmp ioctl request and allow request
only if it fits into a qeth command buffer.

Signed-off-by: Ursula Braun <ursula.braun@de.ibm.com>
Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
Reviewed-by: Heiko Carstens <heicars2@linux.vnet.ibm.com>
Reported-by: Nico Golde <nico@ngolde.de>
Reported-by: Fabian Yamaguchi <fabs@goesec.de>
Cc: <stable@vger.kernel.org>
---
 drivers/s390/net/qeth_core_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index d7b66a2..642f623 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -4451,7 +4451,7 @@ int qeth_snmp_command(struct qeth_card *card, char __user *udata)
 	struct qeth_cmd_buffer *iob;
 	struct qeth_ipa_cmd *cmd;
 	struct qeth_snmp_ureq *ureq;
-	int req_len;
+	unsigned int req_len;
 	struct qeth_arp_query_info qinfo = {0, };
 	int rc = 0;
 
@@ -4467,6 +4467,10 @@ int qeth_snmp_command(struct qeth_card *card, char __user *udata)
 	/* skip 4 bytes (data_len struct member) to get req_len */
 	if (copy_from_user(&req_len, udata + sizeof(int), sizeof(int)))
 		return -EFAULT;
+	if (req_len > QETH_BUFSIZE - IPA_PDU_HEADER_SIZE
+				   - sizeof(struct qeth_ipacmd_hdr)
+				   - sizeof(struct qeth_ipacmd_setadpparms_hdr))
+		return -EINVAL;
 	ureq = memdup_user(udata, req_len + sizeof(struct qeth_snmp_ureq_hdr));
 	if (IS_ERR(ureq)) {
 		QETH_CARD_TEXT(card, 2, "snmpnome");

^ permalink raw reply related

* Re: [patch net-next 3/3] fix skb_morph to preserve skb->sk and skb->destructor pointers
From: Eric Dumazet @ 2013-11-05 14:06 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, pablo, netfilter-devel, yoshfuji, kadlec, kaber,
	mleitner, kuznet, jmorris, wensong, horms, ja, edumazet, pshelar,
	jasowang, alexander.h.duyck, coreteam, fw
In-Reply-To: <1383649333-6321-4-git-send-email-jiri@resnulli.us>

On Tue, 2013-11-05 at 12:02 +0100, Jiri Pirko wrote:
> Currently __skb_clone sets skb->sk and skb->destructor to NULL. This is
> not right for skb_morph use case because skb->sk may be previously
> set (e. g. by xt_TPROXY).
> 
> Also, during skb_morph the destructor should not be called. It might be
> previously set, e. g. by xt_TPROXY to sock_edemux, and that would cause
> put sk while skb is still in flight.

truesize alert. You should add some documentation that skb_morph()
must not be used in transmit path.

Its not clear to be how this can happen.

skb_morph() being used only from ipv4 defrag (or ipv6 reassembly).

Maybe the problem could be fixed by doing this defrag _before_ setting
skb->sk ?

Also, I would prefer you find a way to put all this logic inside
skb_morph() instead of adding such complexity in this already complex
code, I fear the compiler will generate slower code with your patch on
fast path.

Maybe something as simple as following (untested) patch ?

Note that the truesize concern might need some care.

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3735fad5616e..afabfd6ef341 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -793,18 +793,28 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 
 /**
  *	skb_morph	-	morph one skb into another
- *	@dst: the skb to receive the contents
+ *	@skb: the skb to receive the contents
  *	@src: the skb to supply the contents
  *
  *	This is identical to skb_clone except that the target skb is
- *	supplied by the user.
+ *	supplied by the user, and that we keep target skb destructor in place,
+ *	meaning this can not be used in transmit path, as skb->truesize might
+ *	change.
  *
  *	The target skb is returned upon exit.
  */
-struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src)
+struct sk_buff *skb_morph(struct sk_buff *skb, struct sk_buff *src)
 {
-	skb_release_all(dst);
-	return __skb_clone(dst, src);
+	struct sock *save_sk = skb->sk;
+	void (*save_destructor)(struct sk_buff *) = skb->destructor;
+
+	skb->sk = NULL;
+	skb->destructor = NULL;
+	skb_release_all(skb);
+	__skb_clone(skb, src);
+	skb->sk = save_sk;
+	skb->destructor = save_destructor;
+	return skb;
 }
 EXPORT_SYMBOL_GPL(skb_morph);
 



^ permalink raw reply related

* [PATCH net-next 3/3] net: allow to leave the buffer fragmented in skb_cow_data()
From: Mathias Krause @ 2013-11-05 13:54 UTC (permalink / raw)
  To: David S. Miller, Steffen Klassert, Herbert Xu
  Cc: Dmitry Tarnyagin, netdev, Mathias Krause, David S. Miller,
	Herbert Xu
In-Reply-To: <cover.1381923854.git.mathias.krause@secunet.com>

Do not linearize the buffer per se but only if we're expected to expand
the tail. All callers can handle fragmented buffers and even expect
them!

Not linearizing the buffer leads to a small performance improvement for
the IPsec receive path in case the network driver passed us a fragmented
buffer.

With this patch applied I was able to increase the throughput of an
IPsec gateway from 7.12 Gbit/s to 7.28 Gbit/s.

Signed-off-by: Mathias Krause <mathias.krause@secunet.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 net/core/skbuff.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 247023d..5eec1b9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3207,7 +3207,7 @@ EXPORT_SYMBOL_GPL(skb_to_sgvec);
  *
  *	If @tailbits is given, make sure that there is space to write @tailbits
  *	bytes of data beyond current end of socket buffer.  @trailer will be
- *	set to point to the skb in which this space begins.
+ *	linearized and set to point to the skb in which this space begins.
  *
  *	The number of scatterlist elements required to completely map the
  *	COW'd and extended socket buffer will be returned.
@@ -3218,11 +3218,10 @@ int skb_cow_data(struct sk_buff *skb, int tailbits, struct sk_buff **trailer)
 	int elt;
 	struct sk_buff *skb1, **skb_p;
 
-	/* If skb is cloned or its head is paged, reallocate
-	 * head pulling out all the pages (pages are considered not writable
-	 * at the moment even if they are anonymous).
+	/* If skb is cloned reallocate head pulling out all the pages (pages are
+	 * considered not writable at the moment even if they are anonymous).
 	 */
-	if ((skb_cloned(skb) || skb_shinfo(skb)->nr_frags) &&
+	if (skb_cloned(skb) &&
 	    __pskb_pull_tail(skb, skb_pagelen(skb)-skb_headlen(skb)) == NULL)
 		return -ENOMEM;
 
@@ -3233,18 +3232,26 @@ int skb_cow_data(struct sk_buff *skb, int tailbits, struct sk_buff **trailer)
 		 * good frames. OK, on miss we reallocate and reserve even more
 		 * space, 128 bytes is fair. */
 
-		if (skb_tailroom(skb) < tailbits &&
-		    pskb_expand_head(skb, 0, tailbits-skb_tailroom(skb)+128, GFP_ATOMIC))
-			return -ENOMEM;
+		if (tailbits) {
+			if (skb_linearize(skb))
+				return -ENOMEM;
+
+			if (skb_tailroom(skb) < tailbits) {
+				int ntail = tailbits - skb_tailroom(skb) + 128;
+
+				if (pskb_expand_head(skb, 0, ntail, GFP_ATOMIC))
+					return -ENOMEM;
+			}
+		}
 
 		/* Voila! */
 		*trailer = skb;
-		return 1;
+		return skb_shinfo(skb)->nr_frags + 1;
 	}
 
 	/* Misery. We are in troubles, going to mincer fragments... */
 
-	elt = 1;
+	elt = skb_shinfo(skb)->nr_frags + 1;
 	skb_p = &skb_shinfo(skb)->frag_list;
 	copyflag = 0;
 
@@ -3296,7 +3303,7 @@ int skb_cow_data(struct sk_buff *skb, int tailbits, struct sk_buff **trailer)
 			kfree_skb(skb1);
 			skb1 = skb2;
 		}
-		elt++;
+		elt += skb_shinfo(skb1)->nr_frags + 1;
 		*trailer = skb1;
 		skb_p = &skb1->next;
 	}
-- 
1.7.2.5

^ permalink raw reply related

* [PATCH net-next 1/3] net: move pskb_put() to core code
From: Mathias Krause @ 2013-11-05 13:54 UTC (permalink / raw)
  To: David S. Miller, Steffen Klassert, Herbert Xu
  Cc: Dmitry Tarnyagin, netdev, Mathias Krause, Steffen Klassert,
	David S. Miller, Herbert Xu
In-Reply-To: <cover.1381923854.git.mathias.krause@secunet.com>

This function has usage beside IPsec so move it to the core skbuff code.
While doing so, give it some documentation and change its return type to
'unsigned char *' to be in line with skb_put().

Signed-off-by: Mathias Krause <mathias.krause@secunet.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 include/linux/skbuff.h |    1 +
 include/net/esp.h      |    2 --
 net/core/skbuff.c      |   23 +++++++++++++++++++++++
 net/xfrm/xfrm_algo.c   |   13 -------------
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c2d8933..9d785f4d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1418,6 +1418,7 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset)
 /*
  *	Add data to an sk_buff
  */
+unsigned char *pskb_put(struct sk_buff *skb, struct sk_buff *tail, int len);
 unsigned char *skb_put(struct sk_buff *skb, unsigned int len);
 static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
 {
diff --git a/include/net/esp.h b/include/net/esp.h
index d584513..5b4fdcc 100644
--- a/include/net/esp.h
+++ b/include/net/esp.h
@@ -13,8 +13,6 @@ struct esp_data {
 
 #include <linux/skbuff.h>
 
-void *pskb_put(struct sk_buff *skb, struct sk_buff *tail, int len);
-
 struct ip_esp_hdr;
 
 static inline struct ip_esp_hdr *ip_esp_hdr(const struct sk_buff *skb)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d81cff1..247023d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1257,6 +1257,29 @@ free_skb:
 EXPORT_SYMBOL(skb_pad);
 
 /**
+ *	pskb_put - add data to the tail of a potentially fragmented buffer
+ *	@skb: start of the buffer to use
+ *	@tail: tail fragment of the buffer to use
+ *	@len: amount of data to add
+ *
+ *	This function extends the used data area of the potentially
+ *	fragmented buffer. &tail must be the last fragment of &skb -- or
+ *	&skb itself. If this would exceed the total buffer size the kernel
+ *	will panic. A pointer to the first byte of the extra data is
+ *	returned.
+ */
+
+unsigned char *pskb_put(struct sk_buff *skb, struct sk_buff *tail, int len)
+{
+	if (tail != skb) {
+		skb->data_len += len;
+		skb->len += len;
+	}
+	return skb_put(tail, len);
+}
+EXPORT_SYMBOL_GPL(pskb_put);
+
+/**
  *	skb_put - add data to a buffer
  *	@skb: buffer to use
  *	@len: amount of data to add
diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
index ab4ef72..debe733 100644
--- a/net/xfrm/xfrm_algo.c
+++ b/net/xfrm/xfrm_algo.c
@@ -802,17 +802,4 @@ int xfrm_count_pfkey_enc_supported(void)
 }
 EXPORT_SYMBOL_GPL(xfrm_count_pfkey_enc_supported);
 
-#if defined(CONFIG_INET_ESP) || defined(CONFIG_INET_ESP_MODULE) || defined(CONFIG_INET6_ESP) || defined(CONFIG_INET6_ESP_MODULE)
-
-void *pskb_put(struct sk_buff *skb, struct sk_buff *tail, int len)
-{
-	if (tail != skb) {
-		skb->data_len += len;
-		skb->len += len;
-	}
-	return skb_put(tail, len);
-}
-EXPORT_SYMBOL_GPL(pskb_put);
-#endif
-
 MODULE_LICENSE("GPL");
-- 
1.7.2.5

^ permalink raw reply related

* [PATCH net-next 2/3] caif: use pskb_put() instead of reimplementing its functionality
From: Mathias Krause @ 2013-11-05 13:54 UTC (permalink / raw)
  To: David S. Miller, Steffen Klassert, Herbert Xu
  Cc: Dmitry Tarnyagin, netdev, Mathias Krause, David S. Miller
In-Reply-To: <cover.1381923854.git.mathias.krause@secunet.com>

Also remove the warning for fragmented packets -- skb_cow_data() will
linearize the buffer, removing all fragments.

Signed-off-by: Mathias Krause <mathias.krause@secunet.com>
Cc: Dmitry Tarnyagin <dmitry.tarnyagin@lockless.no>
Cc: "David S. Miller" <davem@davemloft.net>
---
 net/caif/cfpkt_skbuff.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/net/caif/cfpkt_skbuff.c b/net/caif/cfpkt_skbuff.c
index 6493351..1be0b52 100644
--- a/net/caif/cfpkt_skbuff.c
+++ b/net/caif/cfpkt_skbuff.c
@@ -203,20 +203,10 @@ int cfpkt_add_body(struct cfpkt *pkt, const void *data, u16 len)
 			PKT_ERROR(pkt, "cow failed\n");
 			return -EPROTO;
 		}
-		/*
-		 * Is the SKB non-linear after skb_cow_data()? If so, we are
-		 * going to add data to the last SKB, so we need to adjust
-		 * lengths of the top SKB.
-		 */
-		if (lastskb != skb) {
-			pr_warn("Packet is non-linear\n");
-			skb->len += len;
-			skb->data_len += len;
-		}
 	}
 
 	/* All set to put the last SKB and optionally write data there. */
-	to = skb_put(lastskb, len);
+	to = pskb_put(skb, lastskb, len);
 	if (likely(data))
 		memcpy(to, data, len);
 	return 0;
-- 
1.7.2.5

^ permalink raw reply related

* [PATCH net-next 0/3] IPsec improvements
From: Mathias Krause @ 2013-11-05 13:54 UTC (permalink / raw)
  To: David S. Miller, Steffen Klassert, Herbert Xu
  Cc: Dmitry Tarnyagin, netdev, Mathias Krause

This series moves pskb_put() to the core code -- making the code
duplication in caif obsolete (patches 1 and 2).

Additionally does this series optimize the IPsec receive path in patch 3
by allowing skb_cow_data() to leave the buffer fragmented. I noticed the
linearization to be a bottleneck when doing some VPN gateway benchmarks.
Linearization of the buffer isn't needed in the receive path as the
crypto API (and all other users of skb_cow_data) can handle sg.

With patch 3 applied I was able to increase the throughput of an IPsec
gateway from 7.12 Gbit/s to 7.28 Gbit/s.


Please apply!

Mathias Krause (3):
  net: move pskb_put() to core code
  caif: use pskb_put() instead of reimplementing its functionality
  net: allow to leave the buffer fragmented in skb_cow_data()

 include/linux/skbuff.h  |    1 +
 include/net/esp.h       |    2 -
 net/caif/cfpkt_skbuff.c |   12 +----------
 net/core/skbuff.c       |   52 +++++++++++++++++++++++++++++++++++++----------
 net/xfrm/xfrm_algo.c    |   13 -----------
 5 files changed, 43 insertions(+), 37 deletions(-)

-- 
1.7.2.5

^ permalink raw reply

* Re: [patch net-next 2/3] netfilter: ip6_tables: use reasm skb for matching
From: Patrick McHardy @ 2013-11-05 13:41 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Jiri Pirko, netdev, davem, pablo, netfilter-devel, yoshfuji,
	kadlec, mleitner, kuznet, jmorris, wensong, horms, ja, edumazet,
	pshelar, jasowang, alexander.h.duyck, coreteam
In-Reply-To: <20131105133205.GC15370@breakpoint.cc>

On Tue, Nov 05, 2013 at 02:32:05PM +0100, Florian Westphal wrote:
> Jiri Pirko <jiri@resnulli.us> wrote:
> > This patch fixes for example following situation:
> > On HOSTA do:
> > ip6tables -I INPUT -p icmpv6 -j DROP
> > ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT
> 
> untested:
> 
> -A INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT
> -A INPUT -p icmpv6 -m conntrack --ctstatus CONFIRMED -j ACCEPT
> -A INPUT -p icmpv6 -j DROP
> 
> > and on HOSTB you do:
> > ping6 HOSTA -s2000    (MTU is 1500)
> > 
> > Incoming echo requests will be filtered out on HOSTA. This issue does
> > not occur with smaller packets than MTU (where fragmentation does not happen).
> 
> Patrick, any reason not to kill the special-casing (ct has assigned helper or
> unconfirmed conntrack) in __ipv6_conntrack_in() ?
> 
> This should make ipv6 frag behaviour consistent; right now its rather
> confusing from ruleset point of view, especially the first packet
> of a connection is always seen as reassembled.
> 
> So with Jiris rules
> 
> -A INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT
> -A INPUT -p icmpv6 -j DROP
> 
> ping6 -s $bignum works for the first packet but not for subsequent ones
> which is quite irritating.

Well, the reason was to avoid unnecessary work doing refragmentation
unless really required. I know its rather complicated, but IPv6 has
always required treating fragments manually or using conntrack state.

I'm not objecting to changing this, but the patches as they are are
not the way to go. First, moving nfct_frag to struct sk_buff seems
like a real waste of space for this quite rare case. Also, we can't
just use the reassembled packet in ip6tables, when modifying it we
will still output the unchanged fragments. An last of all, we'll be
executing the rules on the reassembled packet multiple times, one
for each fragment.

So if someone wants to change this, simply *only* pass the reassembled
packet through the netfilter hooks and drop the fragments, as in IPv4.

^ permalink raw reply

* Re: [patch net-next 2/3] netfilter: ip6_tables: use reasm skb for matching
From: Florian Westphal @ 2013-11-05 13:32 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, pablo, netfilter-devel, yoshfuji, kadlec, kaber,
	mleitner, kuznet, jmorris, wensong, horms, ja, edumazet, pshelar,
	jasowang, alexander.h.duyck, coreteam, fw
In-Reply-To: <1383649333-6321-3-git-send-email-jiri@resnulli.us>

Jiri Pirko <jiri@resnulli.us> wrote:
> This patch fixes for example following situation:
> On HOSTA do:
> ip6tables -I INPUT -p icmpv6 -j DROP
> ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT

untested:

-A INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT
-A INPUT -p icmpv6 -m conntrack --ctstatus CONFIRMED -j ACCEPT
-A INPUT -p icmpv6 -j DROP

> and on HOSTB you do:
> ping6 HOSTA -s2000    (MTU is 1500)
> 
> Incoming echo requests will be filtered out on HOSTA. This issue does
> not occur with smaller packets than MTU (where fragmentation does not happen).

Patrick, any reason not to kill the special-casing (ct has assigned helper or
unconfirmed conntrack) in __ipv6_conntrack_in() ?

This should make ipv6 frag behaviour consistent; right now its rather
confusing from ruleset point of view, especially the first packet
of a connection is always seen as reassembled.

So with Jiris rules

-A INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT
-A INPUT -p icmpv6 -j DROP

ping6 -s $bignum works for the first packet but not for subsequent ones
which is quite irritating.

This change would obviously have userspace visibility (e.g. -m frag
won't work anymore when conntrack is on), but so far I couldn't come
up with a scenario where a legitimate ruleset could break.

^ permalink raw reply

* Re: [PATCH 3/4] wl1251: spi: add vio regulator support
From: Pavel Machek @ 2013-11-05 13:12 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Reichel, Luciano Coelho, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Tony Lindgren, Russell King, John W. Linville, Felipe Balbi,
	Sachin Kamat, Greg Kroah-Hartman, Bill Pemberton,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1382890469-25286-4-git-send-email-sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>

On Sun 2013-10-27 17:14:28, Sebastian Reichel wrote:
> This patch adds support for requesting the regulator powering
> the vio pin.
> 
> The patch also adds the regulator for the all boards using the
> wl1251 in spi mode (only the Nokia N900).
> 
> Signed-off-by: Sebastian Reichel <sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>

Reviewed-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/4] wl1251: move power GPIO handling into the driver
From: Pavel Machek @ 2013-11-05 13:11 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Reichel, Luciano Coelho, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Tony Lindgren, Russell King, John W. Linville, Felipe Balbi,
	Sachin Kamat, Greg Kroah-Hartman, Bill Pemberton,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1382890469-25286-3-git-send-email-sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>

On Sun 2013-10-27 17:14:27, Sebastian Reichel wrote:
> Move the power GPIO handling from the board code into
> the driver. This is a dependency for device tree support.
> 
> Signed-off-by: Sebastian Reichel <sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>

Reviewed-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH net] vti: fix spd lookup: match plaintext pkt, not ipsec pkt
From: Sergei Shtylyov @ 2013-11-05 13:05 UTC (permalink / raw)
  To: Christophe Gouault, Steffen Klassert, David S. Miller
  Cc: Herbert Xu, Saurabh Mohan, netdev
In-Reply-To: <1383646612-30103-1-git-send-email-christophe.gouault@6wind.com>

Hello.

On 05-11-2013 14:16, Christophe Gouault wrote:

> The vti interface inbound and outbound SPD lookups are based on the
> ipsec packet instead of the plaintext packet.

> Not only is it counterintuitive, it also restricts vti interfaces
> to a single policy (whose selector must match the tunnel local and
> remote addresses).

> The policy selector is supposed to match the plaintext packet, before
> encryption or after decryption.

> This patch performs the SPD lookup based on the plaintext packet. It
> enables to create several polices bound to the vti interface (via a
> mark equal to the vti interface okey).

> It remains possible to apply the same policy to all packets entering
> the vti interface, by setting an any-to-any selector (src 0.0.0.0/0
> dst 0.0.0.0/0 proto any mark OKEY).

> Signed-off-by: Christophe Gouault <christophe.gouault@6wind.com>
> ---
>   net/ipv4/ip_vti.c |   28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)

> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index 6e87f85..a7e03c0 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
[...]
> @@ -133,7 +134,12 @@ static int vti_rcv(struct sk_buff *skb)
>   		 * only match policies with this mark.
>   		 */
>   		skb->mark = be32_to_cpu(tunnel->parms.o_key);
> +		/* the packet is decrypted, but not yet decapsulated.
> +		 * Temporarily make network_header point to the inner header
> +		 * for policy check */

    Multi-line comment style in the networking code is:

/* bla
  * bla
  */

[...]
> @@ -173,17 +181,35 @@ static netdev_tx_t vti_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
>
>   	tos = old_iph->tos;
>
> +	/* SPD lookup: we must provide a dst_entry to xfrm_lookup, normally the
> +	 * route to the final destination. However this route is a route via
> +	 * the vti interface. Now vti interfaces typically have the NOXFRM
> +	 * flag, hence xfrm_lookup would bypass IPsec.
> +	 *
> +	 * Therefore, we feed xfrm_lookup with a route to the vti tunnel remote
> +	 * endpoint instead.
> +	 */

    Hm, you got it right the second and third time.

WBR, Sergei

^ permalink raw reply

* Re: [PATCH nf-next] netfilter: xtables: lightweight process control group matching
From: Daniel Borkmann @ 2013-11-05 13:03 UTC (permalink / raw)
  To: pablo-Cap9r6Oaw4JrovVCs/uTlw
  Cc: netfilter-devel-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA, Tejun Heo,
	cgroups-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <ee0fb538d6e43e23d0488d3edd741de9c4589fb1.1382101225.git.dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

On 10/18/2013 03:28 PM, Daniel Borkmann wrote:
> It would be useful e.g. in a server or desktop environment to have
> a facility in the notion of fine-grained "per application" or "per
> application group" firewall policies. Probably, users in the mobile/
> embedded area (e.g. Android based) with different security policy
> requirements for application groups could have great benefit from
> that as well. For example, with a little bit of configuration effort,
> an admin could whitelist well-known applications, and thus block
> otherwise unwanted "hard-to-track" applications like [1] from a
> user's machine.
>
> Implementation of PID-based matching would not be appropriate
> as they frequently change, and child tracking would make that
> even more complex and ugly. Cgroups would be a perfect candidate
> for accomplishing that as they associate a set of tasks with a
> set of parameters for one or more subsystems, in our case the
> netfilter subsystem, which, of course, can be combined with other
> cgroup subsystems into something more complex.
>
> As mentioned, to overcome this constraint, such processes could
> be placed into one or multiple cgroups where different fine-grained
> rules can be defined depending on the application scenario, while
> e.g. everything else that is not part of that could be dropped (or
> vice versa), thus making life harder for unwanted processes to
> communicate to the outside world. So, we make use of cgroups here
> to track jobs and limit their resources in terms of iptables
> policies; in other words, limiting what they are allowed to
> communicate.
>
> Minimal, basic usage example (many other iptables options can be
> applied obviously):
>
>   1) Configuring cgroups:
>
>    mkdir /sys/fs/cgroup/net_filter
>    mount -t cgroup -o net_filter net_filter /sys/fs/cgroup/net_filter
>    mkdir /sys/fs/cgroup/net_filter/0
>    echo 1 > /sys/fs/cgroup/net_filter/0/net_filter.fwid
>
>   2) Configuring netfilter:
>
>    iptables -A OUTPUT -m cgroup ! --cgroup 1 -j DROP
>
>   3) Running applications:
>
>    ping 208.67.222.222  <pid:1799>
>    echo 1799 > /sys/fs/cgroup/net_filter/0/tasks
>    64 bytes from 208.67.222.222: icmp_seq=44 ttl=49 time=11.9 ms
>    ...
>
>    ping 208.67.220.220  <pid:1804>
>    ping: sendmsg: Operation not permitted
>    ...
>    echo 1804 > /sys/fs/cgroup/net_filter/0/tasks
>    64 bytes from 208.67.220.220: icmp_seq=89 ttl=56 time=19.0 ms
>    ...
>
> Of course, real-world deployments would make use of cgroups user
> space toolsuite, or own custom policy daemons dynamically moving
> applications from/to various net_filter cgroups.
>
> Design considerations appendix:
>
> Based on the discussion from [2], [3], it seems the best tradeoff
> imho to make this a subsystem, here's why:
>
> netfilter is a large enough and ubiquitous subsystem, meaning it
> is not somewhere in a niche, and enabled/shipped on most machines.
> It is true that the descision making on fwid is "outsourced" to
> netfilter itself, but that does not necessarily need to be
> considered as a bad thing to delegate and reuse as much as possible.
> The matching performance in the critical path is just a simple
> comparison of fwid tags, nothing more, thus resulting in a good
> performance suited for high-speed networking. Moreover, by simply
> transfering fwids between user- and kernel space, we can have the
> ruleset as packed as possible, giving an optimal footprint for
> large rulesets using this feature. The alternative draft that we
> have proposed in [3] comes at the cost of exposing some of the
> cgroups internals outside of cgroups to make it work, at least a
> higher memory footprint for transferal of rules and even worse a
> lower performance as more work needs to be done in the matching
> critical path, that is traversing all cgroups a task belongs to
> to find the one of our interest. Moreover, from the usability
> point of view, it seems less intuitive, rather more confusing
> than the approach presented here. Therefore, I consider this design
> the better and less intrusive tradeoff to go with.

As I've provided a code proposal for both variants and a design
discussion/conclusion, are you d'accord with this patch Tejun?

>    [1] http://www.blackhat.com/presentations/bh-europe-06/bh-eu-06-biondi/bh-eu-06-biondi-up.pdf
>    [2] http://patchwork.ozlabs.org/patch/280687/
>    [3] http://patchwork.ozlabs.org/patch/282477/
>
> Signed-off-by: Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>   v1->v2:
>    - Updated commit message, rebased
>    - Applied Gao Feng's feedback from [2]
>
>   Note: iptables part is still available in http://patchwork.ozlabs.org/patch/280690/
>
>   Documentation/cgroups/00-INDEX           |   2 +
>   Documentation/cgroups/net_filter.txt     |  27 +++++
>   include/linux/cgroup_subsys.h            |   5 +
>   include/net/netfilter/xt_cgroup.h        |  58 ++++++++++
>   include/net/sock.h                       |   3 +
>   include/uapi/linux/netfilter/Kbuild      |   1 +
>   include/uapi/linux/netfilter/xt_cgroup.h |  11 ++
>   net/core/scm.c                           |   2 +
>   net/core/sock.c                          |  14 +++
>   net/netfilter/Kconfig                    |   8 ++
>   net/netfilter/Makefile                   |   1 +
>   net/netfilter/xt_cgroup.c                | 177 +++++++++++++++++++++++++++++++
>   12 files changed, 309 insertions(+)
>   create mode 100644 Documentation/cgroups/net_filter.txt
>   create mode 100644 include/net/netfilter/xt_cgroup.h
>   create mode 100644 include/uapi/linux/netfilter/xt_cgroup.h
>   create mode 100644 net/netfilter/xt_cgroup.c
>
> diff --git a/Documentation/cgroups/00-INDEX b/Documentation/cgroups/00-INDEX
> index bc461b6..14424d2 100644
> --- a/Documentation/cgroups/00-INDEX
> +++ b/Documentation/cgroups/00-INDEX
> @@ -20,6 +20,8 @@ memory.txt
>   	- Memory Resource Controller; design, accounting, interface, testing.
>   net_cls.txt
>   	- Network classifier cgroups details and usages.
> +net_filter.txt
> +	- Network firewalling (netfilter) cgroups details and usages.
>   net_prio.txt
>   	- Network priority cgroups details and usages.
>   resource_counter.txt
> diff --git a/Documentation/cgroups/net_filter.txt b/Documentation/cgroups/net_filter.txt
> new file mode 100644
> index 0000000..22759e4
> --- /dev/null
> +++ b/Documentation/cgroups/net_filter.txt
> @@ -0,0 +1,27 @@
> +Netfilter cgroup
> +----------------
> +
> +The netfilter cgroup provides an interface to aggregate jobs
> +to a particular netfilter tag, that can be used to apply
> +various iptables/netfilter policies for those jobs in order
> +to limit resources/abilities for network communication.
> +
> +Creating a net_filter cgroups instance creates a net_filter.fwid
> +file. The value of net_filter.fwid is initialized to 0 on
> +default (so only global iptables/netfilter policies apply).
> +You can write a unique decimal fwid tag into net_filter.fwid
> +file, and use that tag along with iptables' --cgroup option.
> +
> +Minimal/basic usage example:
> +
> +1) Configuring cgroup:
> +
> + mkdir /sys/fs/cgroup/net_filter
> + mount -t cgroup -o net_filter net_filter /sys/fs/cgroup/net_filter
> + mkdir /sys/fs/cgroup/net_filter/0
> + echo 1 > /sys/fs/cgroup/net_filter/0/net_filter.fwid
> + echo [pid] > /sys/fs/cgroup/net_filter/0/tasks
> +
> +2) Configuring netfilter:
> +
> + iptables -A OUTPUT -m cgroup ! --cgroup 1 -p tcp --dport 80 -j DROP
> diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
> index b613ffd..ef58217 100644
> --- a/include/linux/cgroup_subsys.h
> +++ b/include/linux/cgroup_subsys.h
> @@ -50,6 +50,11 @@ SUBSYS(net_prio)
>   #if IS_SUBSYS_ENABLED(CONFIG_CGROUP_HUGETLB)
>   SUBSYS(hugetlb)
>   #endif
> +
> +#if IS_SUBSYS_ENABLED(CONFIG_NETFILTER_XT_MATCH_CGROUP)
> +SUBSYS(net_filter)
> +#endif
> +
>   /*
>    * DO NOT ADD ANY SUBSYSTEM WITHOUT EXPLICIT ACKS FROM CGROUP MAINTAINERS.
>    */
> diff --git a/include/net/netfilter/xt_cgroup.h b/include/net/netfilter/xt_cgroup.h
> new file mode 100644
> index 0000000..b2c702f
> --- /dev/null
> +++ b/include/net/netfilter/xt_cgroup.h
> @@ -0,0 +1,58 @@
> +#ifndef _XT_CGROUP_H
> +#define _XT_CGROUP_H
> +
> +#include <linux/types.h>
> +#include <linux/cgroup.h>
> +#include <linux/hardirq.h>
> +#include <linux/rcupdate.h>
> +
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_MATCH_CGROUP)
> +struct cgroup_nf_state {
> +	struct cgroup_subsys_state css;
> +	u32 fwid;
> +};
> +
> +void sock_update_fwid(struct sock *sk);
> +
> +#if IS_BUILTIN(CONFIG_NETFILTER_XT_MATCH_CGROUP)
> +static inline u32 task_fwid(struct task_struct *p)
> +{
> +	u32 fwid;
> +
> +	if (in_interrupt())
> +		return 0;
> +
> +	rcu_read_lock();
> +	fwid = container_of(task_css(p, net_filter_subsys_id),
> +			    struct cgroup_nf_state, css)->fwid;
> +	rcu_read_unlock();
> +
> +	return fwid;
> +}
> +#elif IS_MODULE(CONFIG_NETFILTER_XT_MATCH_CGROUP)
> +static inline u32 task_fwid(struct task_struct *p)
> +{
> +	struct cgroup_subsys_state *css;
> +	u32 fwid = 0;
> +
> +	if (in_interrupt())
> +		return 0;
> +
> +	rcu_read_lock();
> +	css = task_css(p, net_filter_subsys_id);
> +	if (css)
> +		fwid = container_of(css, struct cgroup_nf_state, css)->fwid;
> +	rcu_read_unlock();
> +
> +	return fwid;
> +}
> +#endif
> +#else /* !CONFIG_NETFILTER_XT_MATCH_CGROUP */
> +static inline u32 task_fwid(struct task_struct *p)
> +{
> +	return 0;
> +}
> +
> +#define sock_update_fwid(sk)
> +#endif /* CONFIG_NETFILTER_XT_MATCH_CGROUP */
> +#endif /* _XT_CGROUP_H */
> diff --git a/include/net/sock.h b/include/net/sock.h
> index e3bf213..f7da4b4 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -387,6 +387,9 @@ struct sock {
>   #if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
>   	__u32			sk_cgrp_prioidx;
>   #endif
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_MATCH_CGROUP)
> +	__u32			sk_cgrp_fwid;
> +#endif
>   	struct pid		*sk_peer_pid;
>   	const struct cred	*sk_peer_cred;
>   	long			sk_rcvtimeo;
> diff --git a/include/uapi/linux/netfilter/Kbuild b/include/uapi/linux/netfilter/Kbuild
> index 1749154..94a4890 100644
> --- a/include/uapi/linux/netfilter/Kbuild
> +++ b/include/uapi/linux/netfilter/Kbuild
> @@ -37,6 +37,7 @@ header-y += xt_TEE.h
>   header-y += xt_TPROXY.h
>   header-y += xt_addrtype.h
>   header-y += xt_bpf.h
> +header-y += xt_cgroup.h
>   header-y += xt_cluster.h
>   header-y += xt_comment.h
>   header-y += xt_connbytes.h
> diff --git a/include/uapi/linux/netfilter/xt_cgroup.h b/include/uapi/linux/netfilter/xt_cgroup.h
> new file mode 100644
> index 0000000..43acb7e
> --- /dev/null
> +++ b/include/uapi/linux/netfilter/xt_cgroup.h
> @@ -0,0 +1,11 @@
> +#ifndef _UAPI_XT_CGROUP_H
> +#define _UAPI_XT_CGROUP_H
> +
> +#include <linux/types.h>
> +
> +struct xt_cgroup_info {
> +	__u32 id;
> +	__u32 invert;
> +};
> +
> +#endif /* _UAPI_XT_CGROUP_H */
> diff --git a/net/core/scm.c b/net/core/scm.c
> index b442e7e..f08672a 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -36,6 +36,7 @@
>   #include <net/sock.h>
>   #include <net/compat.h>
>   #include <net/scm.h>
> +#include <net/netfilter/xt_cgroup.h>
>   #include <net/cls_cgroup.h>
>
>
> @@ -290,6 +291,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>   		/* Bump the usage count and install the file. */
>   		sock = sock_from_file(fp[i], &err);
>   		if (sock) {
> +			sock_update_fwid(sock->sk);
>   			sock_update_netprioidx(sock->sk);
>   			sock_update_classid(sock->sk);
>   		}
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 2bd9b3f..524a376 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -125,6 +125,7 @@
>   #include <linux/skbuff.h>
>   #include <net/net_namespace.h>
>   #include <net/request_sock.h>
> +#include <net/netfilter/xt_cgroup.h>
>   #include <net/sock.h>
>   #include <linux/net_tstamp.h>
>   #include <net/xfrm.h>
> @@ -1337,6 +1338,18 @@ void sock_update_netprioidx(struct sock *sk)
>   EXPORT_SYMBOL_GPL(sock_update_netprioidx);
>   #endif
>
> +#if IS_ENABLED(CONFIG_NETFILTER_XT_MATCH_CGROUP)
> +void sock_update_fwid(struct sock *sk)
> +{
> +	u32 fwid;
> +
> +	fwid = task_fwid(current);
> +	if (fwid != sk->sk_cgrp_fwid)
> +		sk->sk_cgrp_fwid = fwid;
> +}
> +EXPORT_SYMBOL(sock_update_fwid);
> +#endif
> +
>   /**
>    *	sk_alloc - All socket objects are allocated here
>    *	@net: the applicable net namespace
> @@ -1363,6 +1376,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
>
>   		sock_update_classid(sk);
>   		sock_update_netprioidx(sk);
> +		sock_update_fwid(sk);
>   	}
>
>   	return sk;
> diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
> index 6e839b6..d276ff4 100644
> --- a/net/netfilter/Kconfig
> +++ b/net/netfilter/Kconfig
> @@ -806,6 +806,14 @@ config NETFILTER_XT_MATCH_BPF
>
>   	  To compile it as a module, choose M here.  If unsure, say N.
>
> +config NETFILTER_XT_MATCH_CGROUP
> +	tristate '"control group" match support'
> +	depends on NETFILTER_ADVANCED
> +	depends on CGROUPS
> +	---help---
> +	Socket/process control group matching allows you to match locally
> +	generated packets based on which control group processes belong to.
> +
>   config NETFILTER_XT_MATCH_CLUSTER
>   	tristate '"cluster" match support'
>   	depends on NF_CONNTRACK
> diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
> index c3a0a12..12f014f 100644
> --- a/net/netfilter/Makefile
> +++ b/net/netfilter/Makefile
> @@ -124,6 +124,7 @@ obj-$(CONFIG_NETFILTER_XT_MATCH_MULTIPORT) += xt_multiport.o
>   obj-$(CONFIG_NETFILTER_XT_MATCH_NFACCT) += xt_nfacct.o
>   obj-$(CONFIG_NETFILTER_XT_MATCH_OSF) += xt_osf.o
>   obj-$(CONFIG_NETFILTER_XT_MATCH_OWNER) += xt_owner.o
> +obj-$(CONFIG_NETFILTER_XT_MATCH_CGROUP) += xt_cgroup.o
>   obj-$(CONFIG_NETFILTER_XT_MATCH_PHYSDEV) += xt_physdev.o
>   obj-$(CONFIG_NETFILTER_XT_MATCH_PKTTYPE) += xt_pkttype.o
>   obj-$(CONFIG_NETFILTER_XT_MATCH_POLICY) += xt_policy.o
> diff --git a/net/netfilter/xt_cgroup.c b/net/netfilter/xt_cgroup.c
> new file mode 100644
> index 0000000..249c7ee
> --- /dev/null
> +++ b/net/netfilter/xt_cgroup.c
> @@ -0,0 +1,177 @@
> +/*
> + * Xtables module to match the process control group.
> + *
> + * Might be used to implement individual "per-application" firewall
> + * policies in contrast to global policies based on control groups.
> + *
> + * (C) 2013 Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> + * (C) 2013 Thomas Graf <tgraf-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/skbuff.h>
> +#include <linux/module.h>
> +#include <linux/file.h>
> +#include <linux/cgroup.h>
> +#include <linux/fdtable.h>
> +#include <linux/netfilter/x_tables.h>
> +#include <linux/netfilter/xt_cgroup.h>
> +#include <net/netfilter/xt_cgroup.h>
> +#include <net/sock.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Xtables: process control group matching");
> +MODULE_ALIAS("ipt_cgroup");
> +MODULE_ALIAS("ip6t_cgroup");
> +
> +static int cgroup_mt_check(const struct xt_mtchk_param *par)
> +{
> +	struct xt_cgroup_info *info = par->matchinfo;
> +
> +	if (info->invert & ~1)
> +		return -EINVAL;
> +
> +	return info->id ? 0 : -EINVAL;
> +}
> +
> +static bool
> +cgroup_mt(const struct sk_buff *skb, struct xt_action_param *par)
> +{
> +	const struct xt_cgroup_info *info = par->matchinfo;
> +
> +	if (skb->sk == NULL)
> +		return false;
> +
> +	return (info->id == skb->sk->sk_cgrp_fwid) ^ info->invert;
> +}
> +
> +static struct xt_match cgroup_mt_reg __read_mostly = {
> +	.name       = "cgroup",
> +	.revision   = 0,
> +	.family     = NFPROTO_UNSPEC,
> +	.checkentry = cgroup_mt_check,
> +	.match      = cgroup_mt,
> +	.matchsize  = sizeof(struct xt_cgroup_info),
> +	.me         = THIS_MODULE,
> +	.hooks      = (1 << NF_INET_LOCAL_OUT) |
> +	              (1 << NF_INET_POST_ROUTING),
> +};
> +
> +static inline struct cgroup_nf_state *
> +css_nf_state(struct cgroup_subsys_state *css)
> +{
> +	return css ? container_of(css, struct cgroup_nf_state, css) : NULL;
> +}
> +
> +static struct cgroup_subsys_state *
> +cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
> +{
> +	struct cgroup_nf_state *cs;
> +
> +	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
> +	if (!cs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return &cs->css;
> +}
> +
> +static int cgroup_css_online(struct cgroup_subsys_state *css)
> +{
> +	struct cgroup_nf_state *cs = css_nf_state(css);
> +	struct cgroup_nf_state *parent = css_nf_state(css_parent(css));
> +
> +	if (parent)
> +		cs->fwid = parent->fwid;
> +
> +	return 0;
> +}
> +
> +static void cgroup_css_free(struct cgroup_subsys_state *css)
> +{
> +	kfree(css_nf_state(css));
> +}
> +
> +static int cgroup_fwid_update(const void *v, struct file *file, unsigned n)
> +{
> +	int err;
> +	struct socket *sock = sock_from_file(file, &err);
> +
> +	if (sock)
> +		sock->sk->sk_cgrp_fwid = (u32)(unsigned long) v;
> +
> +	return 0;
> +}
> +
> +static u64 cgroup_fwid_read(struct cgroup_subsys_state *css,
> +			    struct cftype *cft)
> +{
> +	return css_nf_state(css)->fwid;
> +}
> +
> +static int cgroup_fwid_write(struct cgroup_subsys_state *css,
> +			     struct cftype *cft, u64 id)
> +{
> +	css_nf_state(css)->fwid = (u32) id;
> +
> +	return 0;
> +}
> +
> +static void cgroup_attach(struct cgroup_subsys_state *css,
> +			  struct cgroup_taskset *tset)
> +{
> +	struct cgroup_nf_state *cs = css_nf_state(css);
> +	void *v = (void *)(unsigned long) cs->fwid;
> +	struct task_struct *p;
> +
> +	cgroup_taskset_for_each(p, css, tset) {
> +		task_lock(p);
> +		iterate_fd(p->files, 0, cgroup_fwid_update, v);
> +		task_unlock(p);
> +	}
> +}
> +
> +static struct cftype net_filter_ss_files[] = {
> +	{
> +		.name		= "fwid",
> +		.read_u64	= cgroup_fwid_read,
> +		.write_u64	= cgroup_fwid_write,
> +	},
> +	{ }
> +};
> +
> +struct cgroup_subsys net_filter_subsys = {
> +	.name		= "net_filter",
> +	.css_alloc	= cgroup_css_alloc,
> +	.css_online	= cgroup_css_online,
> +	.css_free	= cgroup_css_free,
> +	.attach		= cgroup_attach,
> +	.subsys_id	= net_filter_subsys_id,
> +	.base_cftypes	= net_filter_ss_files,
> +	.module		= THIS_MODULE,
> +};
> +
> +static int __init cgroup_mt_init(void)
> +{
> +	int ret = cgroup_load_subsys(&net_filter_subsys);
> +	if (ret)
> +		goto out;
> +
> +	ret = xt_register_match(&cgroup_mt_reg);
> +	if (ret)
> +		cgroup_unload_subsys(&net_filter_subsys);
> +out:
> +	return ret;
> +}
> +
> +static void __exit cgroup_mt_exit(void)
> +{
> +	xt_unregister_match(&cgroup_mt_reg);
> +	cgroup_unload_subsys(&net_filter_subsys);
> +}
> +
> +module_init(cgroup_mt_init);
> +module_exit(cgroup_mt_exit);
>

^ permalink raw reply

* [PATCH net-next 0/2] bonding: extend round-robin mode
From: Nikolay Aleksandrov @ 2013-11-05 12:51 UTC (permalink / raw)
  To: netdev; +Cc: davem, andy, fubar, vfalico

This small patchset adds a new option called packets_per_slave to the
bonding which aims to extend round-robin mode with the following effects:
0 - choose the slave id at random
1 - packet per slave (standard round-robin, default option value)
 >1 - transmit >1 packets per slave, switch the slaves in round-robin
Patch02 adds a description for the new option to the bonding documentation.

Best regards,
 Nikolay Aleksandrov

Nikolay Aleksandrov (2):
  bonding: extend round-robin mode with packets_per_slave
  bonding: document the new packets_per_slave option

 Documentation/networking/bonding.txt |  9 ++++++
 drivers/net/bonding/bond_main.c      | 55 ++++++++++++++++++++++++++++++++----
 drivers/net/bonding/bond_sysfs.c     | 49 ++++++++++++++++++++++++++++++++
 drivers/net/bonding/bonding.h        |  3 +-
 4 files changed, 110 insertions(+), 6 deletions(-)

-- 
1.8.1.4

^ permalink raw reply

* [PATCH net-next 1/2] bonding: extend round-robin mode with packets_per_slave
From: Nikolay Aleksandrov @ 2013-11-05 12:51 UTC (permalink / raw)
  To: netdev; +Cc: davem, andy, fubar, vfalico
In-Reply-To: <1383655902-18744-1-git-send-email-nikolay@redhat.com>

This patch aims to extend round-robin mode with a new option called
packets_per_slave which can have the following values and effects:
0 - choose a random slave
1 (default) - standard round-robin, 1 packet per slave
 >1 - round-robin when >1 packets have been transmitted per slave
The allowed values are between 0 and 65535.
This patch also fixes the comment style in bond_xmit_roundrobin().

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c  | 55 ++++++++++++++++++++++++++++++++++++----
 drivers/net/bonding/bond_sysfs.c | 49 +++++++++++++++++++++++++++++++++++
 drivers/net/bonding/bonding.h    |  3 ++-
 3 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a141f40..4dd5ee2 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -79,6 +79,7 @@
 #include <net/pkt_sched.h>
 #include <linux/rculist.h>
 #include <net/flow_keys.h>
+#include <linux/reciprocal_div.h>
 #include "bonding.h"
 #include "bond_3ad.h"
 #include "bond_alb.h"
@@ -111,6 +112,7 @@ static char *fail_over_mac;
 static int all_slaves_active;
 static struct bond_params bonding_defaults;
 static int resend_igmp = BOND_DEFAULT_RESEND_IGMP;
+static int packets_per_slave = 1;
 
 module_param(max_bonds, int, 0);
 MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
@@ -183,6 +185,10 @@ MODULE_PARM_DESC(all_slaves_active, "Keep all frames received on an interface"
 module_param(resend_igmp, int, 0);
 MODULE_PARM_DESC(resend_igmp, "Number of IGMP membership reports to send on "
 			      "link failure");
+module_param(packets_per_slave, int, 0);
+MODULE_PARM_DESC(packets_per_slave, "Packets to send per slave in balance-rr "
+				    "mode; 0 for a random slave, 1 packet per "
+				    "slave (default), >1 packets per slave.");
 
 /*----------------------------- Global variables ----------------------------*/
 
@@ -3574,14 +3580,44 @@ void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int slave_id)
 	kfree_skb(skb);
 }
 
+/**
+ * bond_rr_gen_slave_id - generate slave id based on packets_per_slave
+ * @bond: bonding device to use
+ *
+ * Based on the value of the bonding device's packets_per_slave parameter
+ * this function generates a slave id, which is usually used as the next
+ * slave to transmit through.
+ */
+static u32 bond_rr_gen_slave_id(struct bonding *bond)
+{
+	int packets_per_slave = bond->params.packets_per_slave;
+	u32 slave_id;
+
+	switch (packets_per_slave) {
+	case 0:
+		slave_id = prandom_u32();
+		break;
+	case 1:
+		slave_id = bond->rr_tx_counter;
+		break;
+	default:
+		slave_id = reciprocal_divide(bond->rr_tx_counter,
+					     packets_per_slave);
+		break;
+	}
+	bond->rr_tx_counter++;
+
+	return slave_id;
+}
+
 static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct iphdr *iph = ip_hdr(skb);
 	struct slave *slave;
+	u32 slave_id;
 
-	/*
-	 * Start with the curr_active_slave that joined the bond as the
+	/* Start with the curr_active_slave that joined the bond as the
 	 * default for sending IGMP traffic.  For failover purposes one
 	 * needs to maintain some consistency for the interface that will
 	 * send the join/membership reports.  The curr_active_slave found
@@ -3594,8 +3630,8 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
 		else
 			bond_xmit_slave_id(bond, skb, 0);
 	} else {
-		bond_xmit_slave_id(bond, skb,
-				   bond->rr_tx_counter++ % bond->slave_cnt);
+		slave_id = bond_rr_gen_slave_id(bond);
+		bond_xmit_slave_id(bond, skb, slave_id % bond->slave_cnt);
 	}
 
 	return NETDEV_TX_OK;
@@ -4099,6 +4135,12 @@ static int bond_check_params(struct bond_params *params)
 		resend_igmp = BOND_DEFAULT_RESEND_IGMP;
 	}
 
+	if (packets_per_slave < 0 || packets_per_slave > USHRT_MAX) {
+		pr_warn("Warning: packets_per_slave (%d) should be between 0 and %u resetting to 1\n",
+			packets_per_slave, USHRT_MAX);
+		packets_per_slave = 1;
+	}
+
 	/* reset values for TLB/ALB */
 	if ((bond_mode == BOND_MODE_TLB) ||
 	    (bond_mode == BOND_MODE_ALB)) {
@@ -4288,7 +4330,10 @@ static int bond_check_params(struct bond_params *params)
 	params->resend_igmp = resend_igmp;
 	params->min_links = min_links;
 	params->lp_interval = BOND_ALB_DEFAULT_LP_INTERVAL;
-
+	if (packets_per_slave > 1)
+		params->packets_per_slave = reciprocal_value(packets_per_slave);
+	else
+		params->packets_per_slave = packets_per_slave;
 	if (primary) {
 		strncpy(params->primary, primary, IFNAMSIZ);
 		params->primary[IFNAMSIZ - 1] = 0;
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 47749c9..75dc4d0 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -40,6 +40,7 @@
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
 #include <linux/nsproxy.h>
+#include <linux/reciprocal_div.h>
 
 #include "bonding.h"
 
@@ -1640,6 +1641,53 @@ out:
 static DEVICE_ATTR(lp_interval, S_IRUGO | S_IWUSR,
 		   bonding_show_lp_interval, bonding_store_lp_interval);
 
+static ssize_t bonding_show_packets_per_slave(struct device *d,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct bonding *bond = to_bond(d);
+	int packets_per_slave = bond->params.packets_per_slave;
+
+	if (packets_per_slave > 1)
+		packets_per_slave = reciprocal_value(packets_per_slave);
+
+	return sprintf(buf, "%d\n", packets_per_slave);
+}
+
+static ssize_t bonding_store_packets_per_slave(struct device *d,
+					       struct device_attribute *attr,
+					       const char *buf, size_t count)
+{
+	struct bonding *bond = to_bond(d);
+	int new_value, ret = count;
+
+	if (sscanf(buf, "%d", &new_value) != 1) {
+		pr_err("%s: no packets_per_slave value specified.\n",
+		       bond->dev->name);
+		ret = -EINVAL;
+		goto out;
+	}
+	if (new_value < 0 || new_value > USHRT_MAX) {
+		pr_err("%s: packets_per_slave must be between 0 and %u\n",
+		       bond->dev->name, USHRT_MAX);
+		ret = -EINVAL;
+		goto out;
+	}
+	if (bond->params.mode != BOND_MODE_ROUNDROBIN)
+		pr_warn("%s: Warning: packets_per_slave has effect only in balance-rr mode\n",
+			bond->dev->name);
+	if (new_value > 1)
+		bond->params.packets_per_slave = reciprocal_value(new_value);
+	else
+		bond->params.packets_per_slave = new_value;
+out:
+	return ret;
+}
+
+static DEVICE_ATTR(packets_per_slave, S_IRUGO | S_IWUSR,
+		   bonding_show_packets_per_slave,
+		   bonding_store_packets_per_slave);
+
 static struct attribute *per_bond_attrs[] = {
 	&dev_attr_slaves.attr,
 	&dev_attr_mode.attr,
@@ -1671,6 +1719,7 @@ static struct attribute *per_bond_attrs[] = {
 	&dev_attr_resend_igmp.attr,
 	&dev_attr_min_links.attr,
 	&dev_attr_lp_interval.attr,
+	&dev_attr_packets_per_slave.attr,
 	NULL,
 };
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 046a605..77a07a1 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -156,6 +156,7 @@ struct bond_params {
 	int all_slaves_active;
 	int resend_igmp;
 	int lp_interval;
+	int packets_per_slave;
 };
 
 struct bond_parm_tbl {
@@ -222,7 +223,7 @@ struct bonding {
 	char     proc_file_name[IFNAMSIZ];
 #endif /* CONFIG_PROC_FS */
 	struct   list_head bond_list;
-	u16      rr_tx_counter;
+	u32      rr_tx_counter;
 	struct   ad_bond_info ad_info;
 	struct   alb_bond_info alb_info;
 	struct   bond_params params;
-- 
1.8.1.4

^ permalink raw reply related

* [PATCH net-next 2/2] bonding: document the new packets_per_slave option
From: Nikolay Aleksandrov @ 2013-11-05 12:51 UTC (permalink / raw)
  To: netdev; +Cc: davem, andy, fubar, vfalico
In-Reply-To: <1383655902-18744-1-git-send-email-nikolay@redhat.com>

Add new documentation for the packets_per_slave option available
for balance-rr mode.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 Documentation/networking/bonding.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 3856ed2..2cdb8b6 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -639,6 +639,15 @@ num_unsol_na
 	are generated by the ipv4 and ipv6 code and the numbers of
 	repetitions cannot be set independently.
 
+packets_per_slave
+
+	Specify the number of packets to transmit through a slave before
+	moving to the next one. When set to 0 then a slave is chosen at
+	random.
+
+	The valid range is 0 - 65535; the default value is 1. This option
+	has effect only in balance-rr mode.
+
 primary
 
 	A string (eth0, eth2, etc) specifying which slave is the
-- 
1.8.1.4

^ permalink raw reply related

* RE: [PATCH net v2 2/3] r8152: modify the tx flow
From: hayeswang @ 2013-11-05 12:46 UTC (permalink / raw)
  To: 'David Miller'
  Cc: netdev, 'nic_swsd', linux-kernel, linux-usb
In-Reply-To: <20131104.155320.2172500441669637943.davem@davemloft.net>

 David Miller [mailto:davem@davemloft.net] 
> Sent: Tuesday, November 05, 2013 4:53 AM
> To: Hayeswang
> Cc: netdev@vger.kernel.org; nic_swsd; 
> linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH net v2 2/3] r8152: modify the tx flow
[...]
> The more TX work you push into the workqueue handler, the longer the
> latency for releasing the SKB and releasing all the queues that are
> waiting for release of that packet.
> 
> Do you know that sockets, queueing discplines, etc. all rely upon
> there being a timely release of SKBs once they are successfully
> transmitted?  It must happen at the earliest moment possible that
> can be reasonable obtained.

Thanks for your answer. I would resend the patches after finishing the
setting of the queue length.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox