ip_options_compile cleaning

PATCH In review Denis V. Lunev 4 participants 8 messages Mar 4, 2008 – Mar 19, 2008

Subsystems (4)

Networking Network drivers IPv4/IPv6 Labeled networking (NetLabel)

lore

Share & tools

lore

Denis V. Lunev

author

2008-03-04 12:21 UTC #0 lore raw

cover ip_options_compile cleaning

Dave,

I have separated yesterday patch we have discussed into three parts to make it a bit clearer for the future readers.

Regards,
Den

Denis V. Lunev

author

2008-03-04 12:22 UTC #1 lore raw

PATCH 1/3 : Remove unnecessary check for opt->is_data in ip_options_compile.

There is the only way to reach ip_options compile with opt != NULL:

ip_options_get_finish

    opt->is_data = 1;
    ip_options_compile(opt, NULL)

So, checking for is_data inside opt != NULL branch is not needed.

Signed-off-by Denis V. Lunev <[email protected]>

---
 net/ipv4/ip_options.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

net/ipv4/ip_options.c +1 -2

index baaedd9..d03eec6 100644
@@ -267,8 +267,7 @@ int ip_options_compile(struct ip_options * opt, struct sk_buff * skb)
 		optptr = iph + sizeof(struct iphdr);
 		opt->is_data = 0;
 	} else {
-		optptr = opt->is_data ? opt->__data :
-					(unsigned char *)&(ip_hdr(skb)[1]);
+		optptr = opt->__data;
 		iph = optptr - sizeof(struct iphdr);
 	}

Signature · 1 line

1.5.3.rc5

Paul Moore

2008-03-04 19:03 UTC #2 lore raw

Re: [PATCH 1/3 net-2.6.26] [IPV4]: Remove unnecessary check for opt->is_data in ip_options_compile.

Quoted 18 lines · Denis V. Lunev

There is the only way to reach ip_options compile with opt != NULL:
ip_options_get_finish
    opt->is_data = 1;
    ip_options_compile(opt, NULL)
So, checking for is_data inside opt != NULL branch is not needed.
Signed-off-by Denis V. Lunev <[email protected]>
---
 net/ipv4/ip_options.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
net/ipv4/ip_options.c +0 -0
index baaedd9..d03eec6 100644

net/ipv4/ip_options.c int ip_options_compile(struct ip_options * opt, · line 272

Quoted 8 lines

net/ipv4/ip_options.c +1 -2

@@ -267,8 +267,7 @@ int ip_options_compile(struct ip_options * opt,
 struct sk_buff * skb) optptr = iph + sizeof(struct iphdr);
 		opt->is_data = 0;
 	} else {
-		optptr = opt->is_data ? opt->__data :
-					(unsigned char *)&(ip_hdr(skb)[1]);
+		optptr = opt->__data;
 		iph = optptr - sizeof(struct iphdr);
 	}

I personally think this patch should be combined with path #2; if we aren't going to honor the 'is_data' field we shouldn't give any callers false hope.

Signature · 2 lines

paul moore
linux security @ hp

Denis V. Lunev

author

2008-03-04 12:22 UTC #3 lore raw

PATCH 2/3 : Remove unused ip_options->is_data.

In reply to Denis V. Lunev

ip_options->is_data is assigned only and never checked. The structure is not a part of kernel interface to the user. So, it is safe to remove this field.

Signed-off-by Denis V. Lunev <[email protected]>

---
 include/net/inet_sock.h |    3 +--
 net/ipv4/cipso_ipv4.c   |    1 -
 net/ipv4/ip_options.c   |    5 -----
 3 files changed, 1 insertions(+), 8 deletions(-)

include/net/inet_sock.h +1 -2

index 70013c5..07ce114 100644
@@ -43,8 +43,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,

net/ipv4/cipso_ipv4.c +0 -1

index 8cd357f..4637ded 100644
@@ -1800,7 +1800,6 @@ int cipso_v4_sock_setattr(struct sock *sk,
 	}
 	memcpy(opt->__data, buf, buf_len);
 	opt->optlen = opt_len;
-	opt->is_data = 1;
 	opt->cipso = sizeof(struct iphdr);
 	kfree(buf);
 	buf = NULL;

net/ipv4/ip_options.c +0 -5

index d03eec6..0bf097b 100644
@@ -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) {

@@ -265,7 +262,6 @@ int ip_options_compile(struct ip_options * opt, struct sk_buff * skb)
 		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->__data;
 		iph = optptr - sizeof(struct iphdr);

@@ -519,7 +515,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;

Signature · 1 line

1.5.3.rc5

Paul Moore

2008-03-04 19:08 UTC #4 lore raw

Re: [PATCH 2/3 net-2.6.26] [IPV4]: Remove unused ip_options->is_data.

Quoted 5 lines · Denis V. Lunev

ip_options->is_data is assigned only and never checked. The structure is not a part of kernel interface to the user. So, it is safe to remove this field.
Signed-off-by Denis V. Lunev <[email protected]>

See my previous comment about folding patch #1 into this one. Other than that I don't have any problem with changes from a CIPSO point of view with the current code base. If we need to do something crazy in the future with IPv4 options we can always tackle it then.

Quoted 70 lines

---
 include/net/inet_sock.h |    3 +--
 net/ipv4/cipso_ipv4.c   |    1 -
 net/ipv4/ip_options.c   |    5 -----
 3 files changed, 1 insertions(+), 8 deletions(-)

include/net/inet_sock.h +1 -2

index 70013c5..07ce114 100644
@@ -43,8 +43,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,

net/ipv4/cipso_ipv4.c +0 -1

index 8cd357f..4637ded 100644
@@ -1800,7 +1800,6 @@ int cipso_v4_sock_setattr(struct sock *sk,
 	}
 	memcpy(opt->__data, buf, buf_len);
 	opt->optlen = opt_len;
-	opt->is_data = 1;
 	opt->cipso = sizeof(struct iphdr);
 	kfree(buf);
 	buf = NULL;

net/ipv4/ip_options.c +0 -0

index d03eec6..0bf097b 100644
@@ -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);

net/ipv4/ip_options.c +0 -3

-	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);

net/ipv4/ip_options.c +0 -2

 	if (sopt->optlen == 0) {

@@ -265,7 +262,6 @@ int ip_options_compile(struct ip_options * opt,
 struct sk_buff * skb) 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->__data;
 		iph = optptr - sizeof(struct iphdr);

@@ -519,7 +515,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;

Signature · 2 lines

paul moore
linux security @ hp

Denis V. Lunev

author

2008-03-04 12:22 UTC #5 lore raw

PATCH 3/3 : Always pass ip_options pointer into ip_options_compile.

In reply to Denis V. Lunev

This makes code a bit more uniform and straigthforward.

Signed-off-by Denis V. Lunev <[email protected]>

---
 net/ipv4/ip_input.c   |    5 +++--
 net/ipv4/ip_options.c |   17 +++++++----------
 2 files changed, 10 insertions(+), 12 deletions(-)

net/ipv4/ip_input.c +3 -2

index 6563139..a006c0f 100644
@@ -283,13 +283,14 @@ static inline int ip_rcv_options(struct sk_buff *skb)
 	}
 
 	iph = ip_hdr(skb);
+	opt = &(IPCB(skb)->opt);
+	opt->optlen = iph->ihl*4 - sizeof(struct iphdr);
 
-	if (ip_options_compile(NULL, skb)) {
+	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) {

net/ipv4/ip_options.c +7 ~~-10~~

index 0bf097b..2b45a7d 100644
@@ -255,17 +255,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);
-	} else {
+	struct rtable *rt = NULL;
+
+	if (skb != NULL) {
+		rt = (struct rtable*)skb->dst;
+		optptr = (unsigned char *)&(ip_hdr(skb)[1]);
+	} else
 		optptr = opt->__data;
-		iph = optptr - sizeof(struct iphdr);
-	}
+	iph = optptr - sizeof(struct iphdr);
 
 	for (l = opt->optlen; l > 0; ) {
 		switch (*optptr) {

Signature · 1 line

1.5.3.rc5

Denis V. Lunev

2008-03-19 19:38 UTC #6 lore raw

In reply to Denis V. Lunev

Quoted 2 lines · Denis V. Lunev

I have separated yesterday patch we have discussed into three parts to make it a bit clearer for the future readers.

Dave,

it looks like this set of patches is not gone into the git. Should I re-send them?

Regards,
Den

David Miller

maintainer

2008-03-19 19:55 UTC #7 lore raw

From: "Denis V. Lunev" <[email protected]>
Date: Tue, 18 Mar 2008 17:38:27 +0300

Quoted 8 lines · Denis V. Lunev

Quoted 2 lines
I have separated yesterday patch we have discussed into three parts to make it a bit clearer for the future readers.
Dave,
it looks like this set of patches is not gone into the git. Should I re-send them?

I expected you to resend them, yes.