Re: Race over table->data in proc_do_sync_threshold()

[Julian Anastasov <ja@xxxxxx>]

        Hello,

On Thu, 10 Aug 2023, Sishuai Gong wrote:

> Hello,
> 
> I am not familiar with the code but I would like to give it a try :).
> 
> It seems to me that replacing the second memcpy with WRITE_ONCE() 
> is not necessary as long as we still hold the lock. Otherwise is this close
> to what you suggest?

        Yes, just make it formatted according to
Documentation/process/submitting-patches.rst

        The mutex serializes writers but readers do not use this
mutex. Anyways, looks like kernel provides inline version of
memcpy which works correctly in our case by copying 4-byte integers.

> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> index 62606fb44d02..b4e22e30b896 100644
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> @@ -1876,6 +1876,7 @@ static int
>  proc_do_sync_threshold(struct ctl_table *table, int write,
>                        void *buffer, size_t *lenp, loff_t *ppos)
>  {
> +      struct netns_ipvs *ipvs = table->extra2;
>         int *valp = table->data;
>         int val[2];
>         int rc;
> @@ -1885,6 +1886,7 @@ proc_do_sync_threshold(struct ctl_table *table, int 
> write,
>                 .mode = table->mode,
>         };
> 
> +      mutex_lock(&ipvs->sync_mutex);
>         memcpy(val, valp, sizeof(val));
>         rc = proc_dointvec(&tmp, write, buffer, lenp, ppos);
>         if (write) {
> @@ -1894,6 +1896,7 @@ proc_do_sync_threshold(struct ctl_table *table, int 
> write,
>                 else
>                         memcpy(valp, val, sizeof(val));
>         }
> +      mutex_unlock(&ipvs->sync_mutex);
>         return rc;
>  }
> 
> @@ -4321,6 +4324,7 @@ static int __net_init 
> ip_vs_control_net_init_sysctl(struct netns_ipvs *ipvs)
>         ipvs->sysctl_sync_threshold[0] = DEFAULT_SYNC_THRESHOLD;
>         ipvs->sysctl_sync_threshold[1] = DEFAULT_SYNC_PERIOD;
>         tbl[idx].data = &ipvs->sysctl_sync_threshold;
> +      tbl[idx].extra2 = ipvs;
>         tbl[idx++].maxlen = sizeof(ipvs->sysctl_sync_threshold);
>         ipvs->sysctl_sync_refresh_period = DEFAULT_SYNC_REFRESH_PERIOD;
>         tbl[idx++].data = &ipvs->sysctl_sync_refresh_period;
> 
> > On Aug 10, 2023, at 2:20 AM, Julian Anastasov <ja@xxxxxx> wrote:
> > 
> > 
> > Hello,
> > 
> > On Wed, 9 Aug 2023, Sishuai Gong wrote:
> > 
> >> Hi,
> >> 
> >> We observed races over (struct ctl_table *) table->data when two threads
> >> are running proc_do_sync_threshold() in parallel, as shown below:
> >> 
> >> Thread-1 Thread-2
> >> memcpy(val, valp, sizeof(val)); memcpy(valp, val, sizeof(val));
> >> 
> >> This race probably would mess up table->data. Is it better to add a lock?
> > 
> > We can put mutex_lock(&ipvs->sync_mutex) before the first
> > memcpy and to use two WRITE_ONCE instead of the second memcpy. But
> > this requires extra2 = ipvs in ip_vs_control_net_init_sysctl():
> > 
> > tbl[idx].data = &ipvs->sysctl_sync_threshold;
> > + tbl[idx].extra2 = ipvs;
> > 
> > Will you provide patch?

Regards

--
Julian Anastasov <ja@xxxxxx>