cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL

PATCH Superseded Paul Moore 2 participants 7 messages Nov 20, 2013

Subsystems (3)

Networking Network drivers Labeled networking (NetLabel)

lore

Share & tools

lore

Paul Moore

author

2013-11-20 19:25 UTC #0 lore raw

PATCH 1/1 cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL

Previous commits corrected some problems with cipso_v4_translate() when CONFIG_NETLABEL=n but some additional work is needed to tidy things up a bit.

Signed-off-by Paul Moore <[email protected]>

---
 include/net/cipso_ipv4.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/net/cipso_ipv4.h +1 -1

index a8c2ef6..2244e02 100644
@@ -304,7 +304,7 @@ static inline int cipso_v4_validate(const struct sk_buff *skb,
 
 	for (opt_iter = 6; opt_iter < opt_len;) {
 		tag_len = opt[opt_iter + 1];
-		if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))) {
+		if ((tag_len == 0) || (tag_len > (opt_len - opt_iter))) {
 			err_offset = opt_iter + 1;
 			goto out;
 		}

David Miller

maintainer

2013-11-20 19:34 UTC #1 lore raw

From: Paul Moore <[email protected]>
Date: Wed, 20 Nov 2013 14:25:48 -0500

Quoted 5 lines

Previous commits corrected some problems with cipso_v4_translate() when CONFIG_NETLABEL=n but some additional work is needed to tidy things up a bit.
Signed-off-by Paul Moore <[email protected]>

That's really vague, please describe exactly what is wrong with the existing conditional and how you have fixed it.

Paul Moore

author

2013-11-20 19:45 UTC #2 lore raw

Quoted 11 lines · David Miller

From: Paul Moore <[email protected]>
Date: Wed, 20 Nov 2013 14:25:48 -0500
Quoted 5 lines
Previous commits corrected some problems with cipso_v4_translate() when CONFIG_NETLABEL=n but some additional work is needed to tidy things up a bit.
Signed-off-by Paul Moore <[email protected]>
That's really vague, please describe exactly what is wrong with the existing conditional and how you have fixed it.

I kinda figured the one line patch and "some additional work is needed to tidy things up a bit" summed it up nicely, but I guess not so here ya go ...

First, for reference, here is the diff one more time (some whitespace damage in the paste below for readability):

include/net/cipso_ipv4.h static inline int cipso_v4_validate(...) · line 310

Quoted 13 lines

include/net/cipso_ipv4.h +1 -1

index a8c2ef6..2244e02 100644
@@ -304,7 +304,7 @@ static inline int cipso_v4_validate(...)
  for (opt_iter = 6; opt_iter < opt_len;) {   
      tag_len = opt[opt_iter + 1];
 
-     if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))) {
+     if ((tag_len == 0) || (tag_len > (opt_len - opt_iter))) { 
           err_offset = opt_iter + 1;
           goto out;        
      }

Looking at the original conditional:

if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))

... and the replacement:

if ((tag_len == 0) || (tag_len > (opt_len - opt_iter)))

... we notice that "(opt[opt_iter + 1] > (opt_len - opt_iter))" has been replaced with "(tag_len > (opt_len - opt_iter))", substituting 'tag_len' for 'opt[opt_iter + 1]'. This is acceptable because the the first statement in the for loop is:

tag_len = opt[opt_iter + 1]

... which matches the substitution in the conditional. I'm not sure how much more explicit I can be about this change, it is really pretty minor.

Signature · 2 lines

paul moore
security and virtualization @ redhat

David Miller

maintainer

2013-11-20 19:55 UTC #3 lore raw

From: Paul Moore <[email protected]>
Date: Wed, 20 Nov 2013 14:45:19 -0500

Quoted 13 lines

Looking at the original conditional:
if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))
... and the replacement:
if ((tag_len == 0) || (tag_len > (opt_len - opt_iter)))
... we notice that "(opt[opt_iter + 1] > (opt_len - opt_iter))" has been replaced with "(tag_len > (opt_len - opt_iter))", substituting 'tag_len' for 'opt[opt_iter + 1]'. This is acceptable because the the first statement in the for loop is:

Quoted 4 lines

tag_len = opt[opt_iter + 1]
... which matches the substitution in the conditional. I'm not sure how much more explicit I can be about this change, it is really pretty minor.

Then, two things:

1) This is a cleanup, and therefore not suitable for submission right now because the net-next tree is closed.

2) A more suitable commit log message would have been "Don't needlessly recompute 'opt[opt_iter + 1]' as we already have it stored in tag_len".

Then anyone who reads this commit message can say "yes, obviously this is a correct change and matches what the patch is doing"

Thanks.

Paul Moore

author

2013-11-20 19:59 UTC #4 lore raw

Quoted 16 lines · David Miller

From: Paul Moore <[email protected]>
Date: Wed, 20 Nov 2013 14:45:19 -0500
Quoted 13 lines
Looking at the original conditional:
if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))
... and the replacement:
if ((tag_len == 0) || (tag_len > (opt_len - opt_iter)))
... we notice that "(opt[opt_iter + 1] > (opt_len - opt_iter))" has been replaced with "(tag_len > (opt_len - opt_iter))", substituting 'tag_len' for 'opt[opt_iter + 1]'. This is acceptable because the the first statement in>
the for loop is:
tag_len = opt[opt_iter + 1]

Quoted 7 lines

Quoted 2 lines
... which matches the substitution in the conditional. I'm not sure how much more explicit I can be about this change, it is really pretty minor.
Then, two things:
1) This is a cleanup, and therefore not suitable for submission right now because the net-next tree is closed.

I wasn't expecting it to go into the current merge window, I was just sending it out now so you'd have it for when things started moving again. Think of it as spreading the pain out a bit.

Quoted 5 lines

2) A more suitable commit log message would have been "Don't needlessly recompute 'opt[opt_iter + 1]' as we already have it stored in tag_len".
Then anyone who reads this commit message can say "yes, obviously this is a correct change and matches what the patch is doing"

Fair enough. Do you want a resubmit with your wording once the merge window closes?

Quoted 1 line

Thanks.

Signature · 2 lines

paul moore
security and virtualization @ redhat

David Miller

maintainer

2013-11-20 20:06 UTC #5 lore raw

From: Paul Moore <[email protected]>
Date: Wed, 20 Nov 2013 14:59:33 -0500

Quoted 7 lines · David Miller

Quoted 2 lines
1) This is a cleanup, and therefore not suitable for submission right now because the net-next tree is closed.
I wasn't expecting it to go into the current merge window, I was just sending it out now so you'd have it for when things started moving again. Think of it as spreading the pain out a bit.

When the merge window opens, I make an announcement here on the list politely asking pople to defer net-next patch submissions until after the merge window closes and I explicitly announce that net-next is open again.

Quoted 8 lines

Quoted 5 lines
2) A more suitable commit log message would have been "Don't needlessly recompute 'opt[opt_iter + 1]' as we already have it stored in tag_len".
Then anyone who reads this commit message can say "yes, obviously this is a correct change and matches what the patch is doing"
Fair enough. Do you want a resubmit with your wording once the merge window closes?

... and I announce that net-next is open once more, yes. Thank you.

Paul Moore

author

2013-11-20 20:09 UTC #6 lore raw

Quoted 7 lines · David Miller

From: Paul Moore <[email protected]>
Date: Wed, 20 Nov 2013 14:59:33 -0500
Quoted 4 lines · David Miller
Quoted 2 lines
1) This is a cleanup, and therefore not suitable for submission right now because the net-next tree is closed.

Quoted 8 lines

Quoted 3 lines
I wasn't expecting it to go into the current merge window, I was just sending it out now so you'd have it for when things started moving again.
Think of it as spreading the pain out a bit.
When the merge window opens, I make an announcement here on the list politely asking pople to defer net-next patch submissions until after the merge window closes and I explicitly announce that net-next is open again.

My apologies, I missed that. Subscribed to too many mailing lists and too reliant on my filters to catch the important bits.

Quoted 11 lines

Quoted 9 lines
Quoted 6 lines
2) A more suitable commit log message would have been "Don't needlessly recompute 'opt[opt_iter + 1]' as we already have it stored in tag_len".
Then anyone who reads this commit message can say "yes, obviously this is a correct change and matches what the patch is doing"
Fair enough. Do you want a resubmit with your wording once the merge window closes?
... and I announce that net-next is open once more, yes. Thank you.

Will do.

Signature · 2 lines

paul moore
security and virtualization @ redhat