Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560261 > unrolled thread

[RFC/PATCH 3/3] netfilter: ctnetlink: Fix regression in CTA_HELP processing

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC/PATCH 3/3] netfilter: ctnetlink: Fix regression in CTA_HELP processing Kevin Cernekee <cernekee@chromium.org> - 2017-01-17 06:20 +0100
    Re: [RFC/PATCH 3/3] netfilter: ctnetlink: Fix regression in CTA_HELP processing Doug Anderson <dianders@chromium.org> - 2017-01-18 20:10 +0100

#1560261 — [RFC/PATCH 3/3] netfilter: ctnetlink: Fix regression in CTA_HELP processing

If a user program specifies CTA_HELP but the argument matches the
current conntrack helper name, ignore it instead of generating an error.

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 net/netfilter/nf_conntrack_netlink.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index cc59f388928e..2912f582da65 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1472,14 +1472,19 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
 	struct nlattr *helpinfo = NULL;
 	int err;
 
-	/* don't change helper of sibling connections */
-	if (ct->master)
-		return -EBUSY;
-
 	err = ctnetlink_parse_help(cda[CTA_HELP], &helpname, &helpinfo);
 	if (err < 0)
 		return err;
 
+	/* don't change helper of sibling connections */
+	if (ct->master) {
+		if (help && help->helper &&
+		    !strcmp(help->helper->name, helpname))
+			return 0;
+		else
+			return -EBUSY;
+	}
+
 	if (!strcmp(helpname, "")) {
 		if (help && help->helper) {
 			/* we had a helper before ... */
-- 
2.7.4

[toc] | [next] | [standalone]

#1562075

Hi,

On Mon, Jan 16, 2017 at 9:14 PM, Kevin Cernekee <cernekee@chromium.org> wrote:
> If a user program specifies CTA_HELP but the argument matches the
> current conntrack helper name, ignore it instead of generating an error.

The "subject" of this patch says that it fixes a regression, but that
regression isn't explained anywhere.  The description of this patch
should make it obvious that the regression is the same as described in
your previous patch in this series: "netfilter: ctnetlink: Fix
regression in CTA_TIMEOUT processing".

Specifically: calling this twice with the same value has always
returned an error for the 2nd call, but that error didn't affect
things in quite the same way before commit b7bd1809e078 ("netfilter:
nfnetlink_queue: get rid of nfnetlink_queue_ct.c").


> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>  net/netfilter/nf_conntrack_netlink.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index cc59f388928e..2912f582da65 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1472,14 +1472,19 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
>         struct nlattr *helpinfo = NULL;
>         int err;
>
> -       /* don't change helper of sibling connections */
> -       if (ct->master)
> -               return -EBUSY;
> -
>         err = ctnetlink_parse_help(cda[CTA_HELP], &helpname, &helpinfo);
>         if (err < 0)
>                 return err;
>
> +       /* don't change helper of sibling connections */
> +       if (ct->master) {

As discussed in the Chromium OS review system:

https://chromium-review.googlesource.com/c/428494/1..2/net/netfilter/nf_conntrack_netlink.c

It feels like we need a comment here.  There you added:

* If we try to change the helper to the same thing twice,
* treat the second attempt as a no-op instead of returning
* an error.

That seems like a good comment.

> +               if (help && help->helper &&
> +                   !strcmp(help->helper->name, helpname))

I was also curious: do you need to also compare helpinfo to
helper->to_nlattr() ?

...effectively right now if you call this function twice with the same
value you want to avoid returning an error for the 2nd call, right?
...you just want to treat it as a no-op.  It seems like checking
"helpinfo" would be safer to really detect two calls with the same
value, but maybe I'm being paranoid...

> +                       return 0;
> +               else
> +                       return -EBUSY;
> +       }
> +
>         if (!strcmp(helpname, "")) {
>                 if (help && help->helper) {
>                         /* we had a helper before ... */
> --
> 2.7.4
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web