[PATCH 1/4 net-2.6.26] [IPV4]: skb->dst can't be NULL in ip_options_echo.

[PATCH 1/4 net-2.6.26] [IPV4]: skb->dst can't be NULL in ip_options_echo.

[Denis V. Lunev]

ip_options_echo is called on the packet input path after the initial
routing. The dst entry on the packet is cleared only in the several
very specific places and immidiately assigned back (may be new).

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 net/ipv4/ip_options.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index 4d31515..baaedd9 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -107,10 +107,7 @@ int ip_options_echo(struct ip_options * dopt, struct sk_buff * skb)
 	sptr = skb_network_header(skb);
 	dptr = dopt->__data;
 
-	if (skb->dst)
-		daddr = ((struct rtable*)skb->dst)->rt_spec_dst;
-	else
-		daddr = ip_hdr(skb)->daddr;
+	daddr = ((struct rtable*)skb->dst)->rt_spec_dst;
 
 	if (sopt->rr) {
 		optlen  = sptr[sopt->rr+1];
-- 
1.5.3.rc5

[PATCH 2/4 net-2.6.26] [IPV4]: Cleanup ip_options_compile.

[Denis V. Lunev]

Right now ip_options_compile is called twice as (NULL, skb) and (opt, NULL).
So, let's move opt initialization into caller and remove this initialization
branch.

Additionally, the field ip_options->is_data becomes not needed. All decisions
should be made by the real skb availability.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 include/net/inet_sock.h |    4 +---
 net/ipv4/ip_input.c     |    5 +++--
 net/ipv4/ip_options.c   |   24 +++++++-----------------
 3 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index 70013c5..e63714d 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -29,7 +29,6 @@
 /** struct ip_options - IP Options
  *
  * @faddr - Saved first hop address
- * @is_data - Options in __data, rather than skb
  * @is_strictroute - Strict source route
  * @srr_is_hit - Packet destination addr was our one
  * @is_changed - IP checksum more not valid
@@ -43,8 +42,7 @@ struct ip_options {
 	unsigned char	srr;
 	unsigned char	rr;
 	unsigned char	ts;
-	unsigned char	is_data:1,
-			is_strictroute:1,
+	unsigned char	is_strictroute:1,
 			srr_is_hit:1,
 			is_changed:1,
 			rr_needaddr:1,
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 6563139..3dbdb0c 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -284,12 +284,13 @@ static inline int ip_rcv_options(struct sk_buff *skb)
 
 	iph = ip_hdr(skb);
 
-	if (ip_options_compile(NULL, skb)) {
+	opt = &(IPCB(skb)->opt);
+	opt->optlen = iph->ihl*4 - sizeof(struct iphdr);
+	if (ip_options_compile(opt, skb)) {
 		IP_INC_STATS_BH(IPSTATS_MIB_INHDRERRORS);
 		goto drop;
 	}
 
-	opt = &(IPCB(skb)->opt);
 	if (unlikely(opt->srr)) {
 		struct in_device *in_dev = in_dev_get(dev);
 		if (in_dev) {
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index baaedd9..c49aa79 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -45,7 +45,6 @@ void ip_options_build(struct sk_buff * skb, struct ip_options * opt,
 	memcpy(&(IPCB(skb)->opt), opt, sizeof(struct ip_options));
 	memcpy(iph+sizeof(struct iphdr), opt->__data, opt->optlen);
 	opt = &(IPCB(skb)->opt);
-	opt->is_data = 0;
 
 	if (opt->srr)
 		memcpy(iph+opt->srr+iph[opt->srr+1]-4, &daddr, 4);
@@ -95,8 +94,6 @@ int ip_options_echo(struct ip_options * dopt, struct sk_buff * skb)
 
 	memset(dopt, 0, sizeof(struct ip_options));
 
-	dopt->is_data = 1;
-
 	sopt = &(IPCB(skb)->opt);
 
 	if (sopt->optlen == 0) {
@@ -248,7 +245,6 @@ void ip_options_fragment(struct sk_buff * skb)
 /*
  * Verify options and fill pointers in struct options.
  * Caller should clear *opt, and set opt->data.
- * If opt == NULL, then skb->data should point to IP header.
  */
 
 int ip_options_compile(struct ip_options * opt, struct sk_buff * skb)
@@ -258,19 +254,14 @@ int ip_options_compile(struct ip_options * opt, struct sk_buff * skb)
 	unsigned char * optptr;
 	int optlen;
 	unsigned char * pp_ptr = NULL;
-	struct rtable *rt = skb ? (struct rtable*)skb->dst : NULL;
-
-	if (!opt) {
-		opt = &(IPCB(skb)->opt);
-		iph = skb_network_header(skb);
-		opt->optlen = ((struct iphdr *)iph)->ihl*4 - sizeof(struct iphdr);
-		optptr = iph + sizeof(struct iphdr);
-		opt->is_data = 0;
-	} else {
-		optptr = opt->is_data ? opt->__data :
-					(unsigned char *)&(ip_hdr(skb)[1]);
-		iph = optptr - sizeof(struct iphdr);
+	struct rtable *rt = NULL;
+
+	optptr = opt->__data;
+	if (skb != NULL) {
+		rt = (struct rtable*)skb->dst;
+		optptr = (unsigned char *)&(ip_hdr(skb)[1]);
 	}
+	iph = optptr - sizeof(struct iphdr);
 
 	for (l = opt->optlen; l > 0; ) {
 		switch (*optptr) {
@@ -520,7 +511,6 @@ static int ip_options_get_finish(struct ip_options **optp,
 	while (optlen & 3)
 		opt->__data[optlen++] = IPOPT_END;
 	opt->optlen = optlen;
-	opt->is_data = 1;
 	if (optlen && ip_options_compile(opt, NULL)) {
 		kfree(opt);
 		return -EINVAL;
-- 
1.5.3.rc5

Re: [PATCH 2/4 net-2.6.26] [IPV4]: Cleanup ip_options_compile.

[David Miller]

From: "Denis V. Lunev" <den@openvz.org>
Date: Mon,  3 Mar 2008 18:48:41 +0300

> Right now ip_options_compile is called twice as (NULL, skb) and (opt, NULL).
> So, let's move opt initialization into caller and remove this initialization
> branch.
> 
> Additionally, the field ip_options->is_data becomes not needed. All decisions
> should be made by the real skb availability.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

I don't think this "is_data" elimination works.

Sometimes these option blobs come from the user or elsewhere.  And
that's why we have to sometimes use opt->__data instead of the SKB
embedded blob.

You also missed the is_data case in cipso_ipv4.c, so this wouldn't
even compile.  Is there something wrong with "git grep" on your
computer?

Re: [PATCH 2/4 net-2.6.26] [IPV4]: Cleanup ip_options_compile.

[Denis V. Lunev]

On Mon, 2008-03-03 at 11:55 -0800, David Miller wrote:
> From: "Denis V. Lunev" <den@openvz.org>
> Date: Mon,  3 Mar 2008 18:48:41 +0300
> 
> > Right now ip_options_compile is called twice as (NULL, skb) and (opt, NULL).
> > So, let's move opt initialization into caller and remove this initialization
> > branch.
> > 
> > Additionally, the field ip_options->is_data becomes not needed. All decisions
> > should be made by the real skb availability.
> > 
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> 
> I don't think this "is_data" elimination works.
> 
> Sometimes these option blobs come from the user or elsewhere.  And
> that's why we have to sometimes use opt->__data instead of the SKB
> embedded blob.
> 
> You also missed the is_data case in cipso_ipv4.c, so this wouldn't
> even compile.  Is there something wrong with "git grep" on your
> computer?

thanks for pointing this out.

Though, it seems to me, that this structure does not came from
userspace. At least I do not see a way for this. Could you show me how
this can happen?

Re: [PATCH 2/4 net-2.6.26] [IPV4]: Cleanup ip_options_compile.

[David Miller]

From: "Denis V. Lunev" <den@openvz.org>
Date: Mon, 03 Mar 2008 23:54:58 +0300

> Though, it seems to me, that this structure does not came from
> userspace. At least I do not see a way for this. Could you show me how
> this can happen?

Sorry, I meant to talk about cases where the options have
been munged by the input or packet processing path, and
as a result the kernel intends opt->__data to be used.

For example, we might parse the options of an incoming
packet, edit out some things we wish to ignore, and leave
the result in opt->__data for ip_options_compile() to
process.

cipso_v4_sock_setattr() is a similar case.

Look at how ip_options_get_from_user() works, it copies in the
user provided IP options into opt->__data and sets
opt->is_data.  This gets passed down to ip_options_get_finish()
which passes that down to ip_options_compile().

Next, look at ip_cmsg_send(), it calls ip_options_get() which
copies the data into opt->__data and passes this down to
ip_options_get_finish() and thus ip_options_compile.

I guess your ip_options_get_finish() changes handle these
cases, but I cannot prove that the CIPSO is_data setting
ends up being unused.

I suppose it gets used by ip_options_build() for outgoing
packets, which assumes that is_data is always set?

Re: [PATCH 2/4 net-2.6.26] [IPV4]: Cleanup ip_options_compile.

[Denis V. Lunev]

On Mon, 2008-03-03 at 13:24 -0800, David Miller wrote:
> From: "Denis V. Lunev" <den@openvz.org>
> Date: Mon, 03 Mar 2008 23:54:58 +0300
> 
> > Though, it seems to me, that this structure does not came from
> > userspace. At least I do not see a way for this. Could you show me how
> > this can happen?
> 
> Sorry, I meant to talk about cases where the options have
> been munged by the input or packet processing path, and
> as a result the kernel intends opt->__data to be used.
> 
> For example, we might parse the options of an incoming
> packet, edit out some things we wish to ignore, and leave
> the result in opt->__data for ip_options_compile() to
> process.
> 
> cipso_v4_sock_setattr() is a similar case.
> 
> Look at how ip_options_get_from_user() works, it copies in the
> user provided IP options into opt->__data and sets
> opt->is_data.  This gets passed down to ip_options_get_finish()
> which passes that down to ip_options_compile().
> 
> Next, look at ip_cmsg_send(), it calls ip_options_get() which
> copies the data into opt->__data and passes this down to
> ip_options_get_finish() and thus ip_options_compile.
> 
> I guess your ip_options_get_finish() changes handle these
> cases, but I cannot prove that the CIPSO is_data setting
> ends up being unused.
> 
> I suppose it gets used by ip_options_build() for outgoing
> packets, which assumes that is_data is always set?

is_data is checked in the only place in the code, in the
ip_options_compile. It, in turn is called twice in the original code:

ip_options_get_finish
        opt->is_data = 1;    <-- !!!
        if (optlen && ip_options_compile(opt, NULL))

ip_rcv_options
   if (ip_options_compile(NULL, skb)) {

So, (opt && !opt->is_data) is impossible in the orignal code where it is
checked.

Re: [PATCH 2/4 net-2.6.26] [IPV4]: Cleanup ip_options_compile.

[David Miller]

From: "Denis V. Lunev" <den@openvz.org>
Date: Tue, 04 Mar 2008 01:57:31 +0300

> On Mon, 2008-03-03 at 13:24 -0800, David Miller wrote:
> > I suppose it gets used by ip_options_build() for outgoing
> > packets, which assumes that is_data is always set?
> 
> is_data is checked in the only place in the code, in the
> ip_options_compile. It, in turn is called twice in the original code:
> 
> ip_options_get_finish
>         opt->is_data = 1;    <-- !!!
>         if (optlen && ip_options_compile(opt, NULL))
> 
> ip_rcv_options
>    if (ip_options_compile(NULL, skb)) {
> 
> So, (opt && !opt->is_data) is impossible in the orignal code where it is
> checked.

Ok, fixup the cipso build failure and resubmit.

Thanks.

[PATCH 3/4 net-2.6.26] [SCTP]: seq_printf format warning.

[Denis V. Lunev]

sctp_association->hbinterval is unsigned long. Replace %8d with %8ld.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 net/sctp/proc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 82bea30..138ab5f 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -325,7 +325,7 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 		seq_printf(seq,
 			   "%8p %8p %-3d %-3d %-2d %-4d "
 			   "%4d %8d %8d %7d %5lu %-5d %5d "
-			   "%8d %5d %5d %4d %4d %4d %8d ",
+			   "%8ld %5d %5d %4d %4d %4d %8d ",
 			   assoc, sk, sctp_sk(sk)->type, sk->sk_state,
 			   assoc->state, hash,
 			   assoc->assoc_id,
-- 
1.5.3.rc5

Re: [PATCH 3/4 net-2.6.26] [SCTP]: seq_printf format warning.

[YOSHIFUJI Hideaki / 吉藤英明]

In article <1204559323-19953-3-git-send-email-den@openvz.org> (at Mon,  3 Mar 2008 18:48:42 +0300), "Denis V. Lunev" <den@openvz.org> says:

> sctp_association->hbinterval is unsigned long. Replace %8d with %8ld.

Why not %8lu?

--yoshfuji

[PATCH 3/4 net-2.6.26] [SCTP]: seq_printf format warning. (fixed)

[Denis V. Lunev]

sctp_association->hbinterval is unsigned long. Replace %8d with %8ld.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 net/sctp/proc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 82bea30..138ab5f 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -325,7 +325,7 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 		seq_printf(seq,
 			   "%8p %8p %-3d %-3d %-2d %-4d "
 			   "%4d %8d %8d %7d %5lu %-5d %5d "
-			   "%8d %5d %5d %4d %4d %4d %8d ",
+			   "%8lu %5d %5d %4d %4d %4d %8d ",
 			   assoc, sk, sctp_sk(sk)->type, sk->sk_state,
 			   assoc->state, hash,
 			   assoc->assoc_id,
-- 
1.5.3.rc5

Re: [PATCH 3/4 net-2.6.26] [SCTP]: seq_printf format warning. (fixed)

[David Miller]

From: "Denis V. Lunev" <den@openvz.org>
Date: Mon,  3 Mar 2008 20:53:38 +0300

> sctp_association->hbinterval is unsigned long. Replace %8d with %8ld.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
 ...
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index 82bea30..138ab5f 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -325,7 +325,7 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
>  		seq_printf(seq,
>  			   "%8p %8p %-3d %-3d %-2d %-4d "
>  			   "%4d %8d %8d %7d %5lu %-5d %5d "
> -			   "%8d %5d %5d %4d %4d %4d %8d ",
> +			   "%8lu %5d %5d %4d %4d %4d %8d ",
>  			   assoc, sk, sctp_sk(sk)->type, sk->sk_state,
>  			   assoc->state, hash,
>  			   assoc->assoc_id,

Haste makes waste.

You fixed up the patch but didn't update the changelog entry.  I've
fixed it up, but please just take your time when respinning patches to
avoid errors like this.  It is not a race, and you will not be timed.

Re: [PATCH 3/4 net-2.6.26] [SCTP]: seq_printf format warning.

[Vlad Yasevich]

YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <1204559323-19953-3-git-send-email-den@openvz.org> (at Mon,  3 Mar 2008 18:48:42 +0300), "Denis V. Lunev" <den@openvz.org> says:
> 
>> sctp_association->hbinterval is unsigned long. Replace %8d with %8ld.
> 
> Why not %8lu?

Yes, that's more correct considering it trying to display jiffies.

-vlad

> 
> --yoshfuji
> --
> 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
> 

[PATCH 4/4 net-2.6.26] [TCP]: Merge exit paths in tcp_v4_conn_request.

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 net/ipv4/tcp_ipv4.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 3b26f95..3873c4d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1355,8 +1355,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 			    (s32)(peer->tcp_ts - req->ts_recent) >
 							TCP_PAWS_WINDOW) {
 				NET_INC_STATS_BH(LINUX_MIB_PAWSPASSIVEREJECTED);
-				dst_release(dst);
-				goto drop_and_free;
+				goto drop_and_release;
 			}
 		}
 		/* Kill the following clause, if you dislike this way. */
@@ -1376,24 +1375,21 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 				       "request from %u.%u.%u.%u/%u\n",
 				       NIPQUAD(saddr),
 				       ntohs(tcp_hdr(skb)->source));
-			dst_release(dst);
-			goto drop_and_free;
+			goto drop_and_release;
 		}
 
 		isn = tcp_v4_init_sequence(skb);
 	}
 	tcp_rsk(req)->snt_isn = isn;
 
-	if (__tcp_v4_send_synack(sk, req, dst))
+	if (__tcp_v4_send_synack(sk, req, dst) || want_cookie)
 		goto drop_and_free;
 
-	if (want_cookie) {
-		reqsk_free(req);
-	} else {
-		inet_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
-	}
+	inet_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
 	return 0;
 
+drop_and_release:
+	dst_release(dst);
 drop_and_free:
 	reqsk_free(req);
 drop:
-- 
1.5.3.rc5

Re: [PATCH 4/4 net-2.6.26] [TCP]: Merge exit paths in tcp_v4_conn_request.

[David Miller]

From: "Denis V. Lunev" <den@openvz.org>
Date: Mon,  3 Mar 2008 18:48:43 +0300

> Signed-off-by: Denis V. Lunev <den@openvz.org>

Applied, thanks.

Re: [PATCH 1/4 net-2.6.26] [IPV4]: skb->dst can't be NULL in ip_options_echo.

[David Miller]

From: "Denis V. Lunev" <den@openvz.org>
Date: Mon,  3 Mar 2008 18:48:40 +0300

> ip_options_echo is called on the packet input path after the initial
> routing. The dst entry on the packet is cleared only in the several
> very specific places and immidiately assigned back (may be new).
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Applied, thanks.