Message-ID: <CA+HUmGhURzDX9Sa98S65sVGNu5FsvmHsRz15OvqzKQuGXre04g@mail.gmail.com>
Date:	Thu, 12 Jan 2012 12:48:11 -0800
From:	Francesco Ruggeri <fruggeri@...stanetworks.com>
To:	David Miller <davem@...emloft.net>
Cc:	eric.dumazet@...il.com, netdev@...r.kernel.org
Subject: Re: Race condition in ipv6 code

I am not familiar with the process, how can one submit a patch for
inclusion in future releases?

On a related note, and independently of the race condition, the return
value of proc_dointvec() in addrconf_sysctl_forward() should be
checked or incorrect situations like the following could occur:

1) User successfully sets all/forwarding to 1.
2) User successfully sets eth1/forwarding to 0.
3) User tries to set all/forwarding to 0, but proc_dointvec() fails,
so addrconf_fixup_forwarding() is invoked with both old and new value
as 1. addrconf_forward_change() then executes with a new value of 1,
and it sets the value to 1 for all idevs where the value is 0.
So the result of proc_dointvec() failing when writing 0 to
all/forwarding is that the value of eth1/forwarding is changed back to
1.

Thanks,
Francesco Ruggeri


On Wed, Jan 11, 2012 at 10:44 PM, David Miller <davem@...emloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@...il.com>
> Date: Thu, 12 Jan 2012 07:31:51 +0100
>
>> Real question is : why are we using this horrible thing at all
>>
>> if (!rtnl_trylock())
>>       return restart_syscall();
>
> Because:
>
> --------------------
> commit 88af182e389097997c5e2a0b42285b3522796759
> Author: Eric W. Biederman <ebiederm@...ssion.com>
> Date:   Fri Feb 19 13:22:59 2010 +0000
>
>    net: Fix sysctl restarts...
>
>    Yuck.  It turns out that when we restart sysctls we were restarting
>    with the values already changed.  Which unfortunately meant that
>    the second time through we thought there was no change and skipped
>    all kinds of work, despite the fact that there was indeed a change.
>
>    I have fixed this the simplest way possible by restoring the changed
>    values when we restart the sysctl write.
>
>    One of my coworkers spotted this bug when after disabling forwarding
>    on an interface pings were still forwarded.
>
>    Signed-off-by: Eric W. Biederman <ebiederm@...ssion.com>
>    Signed-off-by: David S. Miller <davem@...emloft.net>
>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 040c4f0..26dec2b 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1317,14 +1317,19 @@ static int devinet_sysctl_forward(ctl_table *ctl, int write,
>  {
>        int *valp = ctl->data;
>        int val = *valp;
> +       loff_t pos = *ppos;
>        int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
>
>        if (write && *valp != val) {
>                struct net *net = ctl->extra2;
>
>                if (valp != &IPV4_DEVCONF_DFLT(net, FORWARDING)) {
> -                       if (!rtnl_trylock())
> +                       if (!rtnl_trylock()) {
> +                               /* Restore the original values before restarting */
> +                               *valp = val;
> +                               *ppos = pos;
>                                return restart_syscall();
> +                       }
>                        if (valp == &IPV4_DEVCONF_ALL(net, FORWARDING)) {
>                                inet_forward_change(net);
>                        } else if (*valp) {
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index de7a194..143791d 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -502,8 +502,11 @@ static int addrconf_fixup_forwarding(struct ctl_table *table, int *p, int old)
>        if (p == &net->ipv6.devconf_dflt->forwarding)
>                return 0;
>
> -       if (!rtnl_trylock())
> +       if (!rtnl_trylock()) {
> +               /* Restore the original values before restarting */
> +               *p = old;
>                return restart_syscall();
> +       }
>
>        if (p == &net->ipv6.devconf_all->forwarding) {
>                __s32 newf = net->ipv6.devconf_all->forwarding;
> @@ -4028,12 +4031,15 @@ int addrconf_sysctl_forward(ctl_table *ctl, int write,
>  {
>        int *valp = ctl->data;
>        int val = *valp;
> +       loff_t pos = *ppos;
>        int ret;
>
>        ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
>
>        if (write)
>                ret = addrconf_fixup_forwarding(ctl, valp, val);
> +       if (ret)
> +               *ppos = pos;
>        return ret;
>  }
>
> @@ -4075,8 +4081,11 @@ static int addrconf_disable_ipv6(struct ctl_table *table, int *p, int old)
>        if (p == &net->ipv6.devconf_dflt->disable_ipv6)
>                return 0;
>
> -       if (!rtnl_trylock())
> +       if (!rtnl_trylock()) {
> +               /* Restore the original values before restarting */
> +               *p = old;
>                return restart_syscall();
> +       }
>
>        if (p == &net->ipv6.devconf_all->disable_ipv6) {
>                __s32 newf = net->ipv6.devconf_all->disable_ipv6;
> @@ -4095,12 +4104,15 @@ int addrconf_sysctl_disable(ctl_table *ctl, int write,
>  {
>        int *valp = ctl->data;
>        int val = *valp;
> +       loff_t pos = *ppos;
>        int ret;
>
>        ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
>
>        if (write)
>                ret = addrconf_disable_ipv6(ctl, valp, val);
> +       if (ret)
> +               *ppos = pos;
>        return ret;
>  }
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists