| Message ID | 20131120192548.5616.74526.stgit@localhost |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Paul Moore <pmoore@redhat.com> Date: Wed, 20 Nov 2013 14:25:48 -0500 > 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 <pmoore@redhat.com> That's really vague, please describe exactly what is wrong with the existing conditional and how you have fixed it. -- 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
On Wednesday, November 20, 2013 02:34:07 PM David Miller wrote: > From: Paul Moore <pmoore@redhat.com> > Date: Wed, 20 Nov 2013 14:25:48 -0500 > > > 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 <pmoore@redhat.com> > > 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): > diff --git a/include/net/cipso_ipv4.h b/include/net/cipso_ipv4.h > index a8c2ef6..2244e02 100644 > --- a/include/net/cipso_ipv4.h > +++ b/include/net/cipso_ipv4.h > @@ -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.
From: Paul Moore <pmoore@redhat.com> Date: Wed, 20 Nov 2013 14:45:19 -0500 > 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. 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. -- 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
On Wednesday, November 20, 2013 02:55:11 PM David Miller wrote: > From: Paul Moore <pmoore@redhat.com> > Date: Wed, 20 Nov 2013 14:45:19 -0500 > > > 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. > > 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. > 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? > Thanks.
From: Paul Moore <pmoore@redhat.com> Date: Wed, 20 Nov 2013 14:59:33 -0500 > On Wednesday, November 20, 2013 02:55:11 PM David Miller wrote: >> 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. >> 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. -- 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
On Wednesday, November 20, 2013 03:06:20 PM David Miller wrote: > From: Paul Moore <pmoore@redhat.com> > Date: Wed, 20 Nov 2013 14:59:33 -0500 > > > On Wednesday, November 20, 2013 02:55:11 PM David Miller wrote: > >> 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. My apologies, I missed that. Subscribed to too many mailing lists and too reliant on my filters to catch the important bits. > >> 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.
diff --git a/include/net/cipso_ipv4.h b/include/net/cipso_ipv4.h index a8c2ef6..2244e02 100644 --- a/include/net/cipso_ipv4.h +++ b/include/net/cipso_ipv4.h @@ -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; }
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 <pmoore@redhat.com> --- include/net/cipso_ipv4.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 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