[fragmentation ICMP 0/2] fragmentation ICMP

[fragmentation ICMP 0/2] fragmentation ICMP

[Andy Zhou]

Currently, we send ICMP packets when errors occur during fragmentation or
de-fragmentation.  However, it is a bug when sending those ICMP packets
in the context of using netfilter for bridging.

Those ICMP packets are only expected in the context of routing, not in
bridging mode.

The Local stack are not involved in bridging forward decisions, thus
should be not used for deciding the reverse path for those ICMP messages.

This bug only affects IPV4, not in IPv6.

This patch series adds a bit in IPCB to control whether ICMP packet
should be generated on fragmentation or de-fragmentation error. Currently
the only user is br_netfilter.

Andy Zhou (2):
  ipv4_fragment: Add a bit in IPCB to control ICMP generation
  bridge: do not send icmp on fragmentation or defragmentation error

 include/net/inet_frag.h   |  2 ++
 include/net/ip.h          |  1 +
 net/bridge/br_netfilter.c |  7 +++++++
 net/ipv4/ip_fragment.c    | 11 ++++++++++-
 net/ipv4/ip_output.c      |  5 +++--
 5 files changed, 23 insertions(+), 3 deletions(-)

-- 
1.9.1

*** BLURB HERE ***

Andy Zhou (2):
  ipv4_fragment: Add a bit in IPCB to control ICMP generation
  bridge: do not send icmp on fragmentation or defragmentation error

 include/net/inet_frag.h   |  2 ++
 include/net/ip.h          |  1 +
 net/bridge/br_netfilter.c |  7 +++++++
 net/ipv4/ip_fragment.c    | 11 ++++++++++-
 net/ipv4/ip_output.c      |  5 +++--
 5 files changed, 23 insertions(+), 3 deletions(-)

-- 
1.9.1

[net-next fragmenation icmp 1/2] ipv4_fragment: Add a bit in IPCB to control ICMP generation

[Andy Zhou]

Currently, on fragmentation or defragmentation error, ICMP error message
can be generated. This is fine when they are used in a routing context,
but does not make sense when they are used in a bridging context, i.e.
netfilter for bridging.

This patch adds a bit in IPCB to control whether ICMP error message
should be generated.

IPV6 fragmentation and defragmentation functions are not reused by
bridge netfilter. Thus, this change is only required for IPV4.

Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 include/net/inet_frag.h |  2 ++
 include/net/ip.h        |  1 +
 net/ipv4/ip_fragment.c  | 11 ++++++++++-
 net/ipv4/ip_output.c    |  5 +++--
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 8d17655..1d57045 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -44,6 +44,7 @@ enum {
  * @meat: length of received fragments so far
  * @flags: fragment queue flags
  * @max_size: (ipv4 only) maximum received fragment size with IP_DF set
+ * @no_icmp:  Do not generate any fragmentation related icmp packet
  * @net: namespace that this frag belongs to
  */
 struct inet_frag_queue {
@@ -58,6 +59,7 @@ struct inet_frag_queue {
 	int			meat;
 	__u8			flags;
 	u16			max_size;
+	bool			no_icmp;
 	struct netns_frags	*net;
 };
 
diff --git a/include/net/ip.h b/include/net/ip.h
index d14af7e..8d81c865 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -45,6 +45,7 @@ struct inet_skb_parm {
 #define IPSKB_FRAG_COMPLETE	BIT(3)
 #define IPSKB_REROUTED		BIT(4)
 #define IPSKB_DOREDIRECT	BIT(5)
+#define IPSKB_NO_FRAG_ICMP	BIT(6)
 
 	u16			frag_max_size;
 };
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index cc1da6d..2f38d30 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -217,7 +217,8 @@ static void ip_expire(unsigned long arg)
 		/* Only an end host needs to send an ICMP
 		 * "Fragment Reassembly Timeout" message, per RFC792.
 		 */
-		if (qp->user == IP_DEFRAG_AF_PACKET ||
+		if (qp->q.no_icmp ||
+		    qp->user == IP_DEFRAG_AF_PACKET ||
 		    ((qp->user >= IP_DEFRAG_CONNTRACK_IN) &&
 		     (qp->user <= __IP_DEFRAG_CONNTRACK_IN_END) &&
 		     (skb_rtable(head)->rt_type != RTN_LOCAL)))
@@ -323,6 +324,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 	int ihl, end;
 	int err = -ENOENT;
 	u8 ecn;
+	bool no_icmp;
 
 	if (qp->q.flags & INET_FRAG_COMPLETE)
 		goto err;
@@ -340,6 +342,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 	offset &= IP_OFFSET;
 	offset <<= 3;		/* offset is in 8-byte chunks */
 	ihl = ip_hdrlen(skb);
+	no_icmp = IPCB(skb)->flags & IPSKB_NO_FRAG_ICMP;
 
 	/* Determine the position of this fragment. */
 	end = offset + skb->len - ihl;
@@ -478,6 +481,8 @@ found:
 	    skb->len + ihl > qp->q.max_size)
 		qp->q.max_size = skb->len + ihl;
 
+	qp->q.no_icmp = qp->q.no_icmp || no_icmp;
+
 	if (qp->q.flags == (INET_FRAG_FIRST_IN | INET_FRAG_LAST_IN) &&
 	    qp->q.meat == qp->q.len) {
 		unsigned long orefdst = skb->_skb_refdst;
@@ -608,6 +613,10 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
 	head->tstamp = qp->q.stamp;
 	IPCB(head)->frag_max_size = qp->q.max_size;
 
+	IPCB(head)->flags &= ~IPSKB_NO_FRAG_ICMP;
+	if (qp->q.no_icmp)
+		IPCB(head)->flags |= IPSKB_NO_FRAG_ICMP;
+
 	iph = ip_hdr(head);
 	/* max_size != 0 implies at least one fragment had IP_DF set */
 	iph->frag_off = qp->q.max_size ? htons(IP_DF) : 0;
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index c65b93a..dfa425c 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -511,8 +511,9 @@ int ip_fragment(struct sock *sk, struct sk_buff *skb,
 		     (IPCB(skb)->frag_max_size &&
 		      IPCB(skb)->frag_max_size > mtu))) {
 		IP_INC_STATS(dev_net(dev), IPSTATS_MIB_FRAGFAILS);
-		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
-			  htonl(mtu));
+		if (!(IPCB(skb)->flags & IPSKB_NO_FRAG_ICMP))
+			icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
+				  htonl(mtu));
 		kfree_skb(skb);
 		return -EMSGSIZE;
 	}
-- 
1.9.1

[net-next fragmenation icmp 2/2] bridge: do not send icmp on fragmentation or defragmentation error

[Andy Zhou]

Set IPSKB_NO_FRAG_ICMP bit when doing IP fragmentation and
defragmentation in bridging mode.

Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 net/bridge/br_netfilter.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index ab55e24..3f3366a 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -663,6 +663,10 @@ static unsigned int br_nf_pre_routing(const struct nf_hook_ops *ops,
 	if (br_parse_ip_options(skb))
 		return NF_DROP;
 
+	/* In case this is a fragmented packet, do not send icmp packet on
+	 * defragmentation error  */
+	IPCB(skb)->flags |= IPSKB_NO_FRAG_ICMP;
+
 	nf_bridge_put(skb->nf_bridge);
 	if (!nf_bridge_alloc(skb))
 		return NF_DROP;
@@ -875,6 +879,9 @@ static int br_nf_dev_queue_xmit(struct sock *sk, struct sk_buff *skb)
 		skb_copy_from_linear_data_offset(skb, -data->size, data->mac,
 						 data->size);
 
+		/* Do not send ICMP packet on fragmentation error */
+		IPCB(skb)->flags |= IPSKB_NO_FRAG_ICMP;
+
 		ret = ip_fragment(sk, skb, br_nf_push_frag_xmit);
 	} else {
 		ret = br_dev_queue_push_xmit(sk, skb);
-- 
1.9.1

Re: [net-next fragmenation icmp 1/2] ipv4_fragment: Add a bit in IPCB to control ICMP generation

[Eric Dumazet]

On Tue, 2015-05-05 at 18:59 -0700, Andy Zhou wrote:
> Currently, on fragmentation or defragmentation error, ICMP error message
> can be generated. This is fine when they are used in a routing context,
> but does not make sense when they are used in a bridging context, i.e.
> netfilter for bridging.
> 
> This patch adds a bit in IPCB to control whether ICMP error message
> should be generated.
> 
> IPV6 fragmentation and defragmentation functions are not reused by
> bridge netfilter. Thus, this change is only required for IPV4.
> 
> Signed-off-by: Andy Zhou <azhou@nicira.com>
> ---
>  include/net/inet_frag.h |  2 ++
>  include/net/ip.h        |  1 +
>  net/ipv4/ip_fragment.c  | 11 ++++++++++-
>  net/ipv4/ip_output.c    |  5 +++--
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
> index 8d17655..1d57045 100644
> --- a/include/net/inet_frag.h
> +++ b/include/net/inet_frag.h
> @@ -44,6 +44,7 @@ enum {
>   * @meat: length of received fragments so far
>   * @flags: fragment queue flags
>   * @max_size: (ipv4 only) maximum received fragment size with IP_DF set
> + * @no_icmp:  Do not generate any fragmentation related icmp packet
>   * @net: namespace that this frag belongs to
>   */
>  struct inet_frag_queue {
> @@ -58,6 +59,7 @@ struct inet_frag_queue {
>  	int			meat;
>  	__u8			flags;
>  	u16			max_size;
> +	bool			no_icmp;
>  	struct netns_frags	*net;
>  };
>  


What about filling the 8bit hole ?

 	int			meat;
  	__u8			flags;
+	bool			no_icmp;
  	u16			max_size;
  	struct netns_frags	*net;

Re: [net-next fragmenation icmp 1/2] ipv4_fragment: Add a bit in IPCB to control ICMP generation

[Andy Zhou]

Since I only need 1 bit for this, I can probably define and use one
more bit in the 'flags' field above.

Thanks for the review.  I will send out a 'v2'.


On Wed, May 6, 2015 at 5:23 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-05-05 at 18:59 -0700, Andy Zhou wrote:
>> Currently, on fragmentation or defragmentation error, ICMP error message
>> can be generated. This is fine when they are used in a routing context,
>> but does not make sense when they are used in a bridging context, i.e.
>> netfilter for bridging.
>>
>> This patch adds a bit in IPCB to control whether ICMP error message
>> should be generated.
>>
>> IPV6 fragmentation and defragmentation functions are not reused by
>> bridge netfilter. Thus, this change is only required for IPV4.
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>> ---
>>  include/net/inet_frag.h |  2 ++
>>  include/net/ip.h        |  1 +
>>  net/ipv4/ip_fragment.c  | 11 ++++++++++-
>>  net/ipv4/ip_output.c    |  5 +++--
>>  4 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
>> index 8d17655..1d57045 100644
>> --- a/include/net/inet_frag.h
>> +++ b/include/net/inet_frag.h
>> @@ -44,6 +44,7 @@ enum {
>>   * @meat: length of received fragments so far
>>   * @flags: fragment queue flags
>>   * @max_size: (ipv4 only) maximum received fragment size with IP_DF set
>> + * @no_icmp:  Do not generate any fragmentation related icmp packet
>>   * @net: namespace that this frag belongs to
>>   */
>>  struct inet_frag_queue {
>> @@ -58,6 +59,7 @@ struct inet_frag_queue {
>>       int                     meat;
>>       __u8                    flags;
>>       u16                     max_size;
>> +     bool                    no_icmp;
>>       struct netns_frags      *net;
>>  };
>>
>
>
> What about filling the 8bit hole ?
>
>         int                     meat;
>         __u8                    flags;
> +       bool                    no_icmp;
>         u16                     max_size;
>         struct netns_frags      *net;
>
>
>