| Message ID | CAM_iQpV-00gemayMkdLGzb8wo8CNimOZQHDXrvH1jeN6Svrs8w@mail.gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2016-10-20 at 12:44 -0700, Cong Wang wrote: > On Thu, Oct 20, 2016 at 12:40 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Thu, Oct 20, 2016 at 12:32 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > >> On Thu, Oct 20, 2016 at 10:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > >>> From: Eric Dumazet <edumazet@google.com> > >>> > >>> In commit 4ee3bd4a8c746 ("ipv4: disable BH when changing ip local port > >>> range") Cong added BH protection in set_local_port_range() but missed > >>> that same fix was needed in set_ping_group_range() > >> > >> Don't know why ping_group_range shares the same lock with local_port_range... > >> Perhaps just for saving a few bytes, but that is why I missed this place. > > > > Hold on... We clearly have typos there... Your fix is not correct. > > We need the attached patch, your patch should be reverted, because > unlike local_port_range we never read it in BH context, no need to bother _bh. Well, we do not change this sysctl very often, so I am not sure why we need different seqlocks to protect these ranges. Seems a waste of space really (per netns)
On Thu, Oct 20, 2016 at 1:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Thu, 2016-10-20 at 12:44 -0700, Cong Wang wrote: >> On Thu, Oct 20, 2016 at 12:40 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > On Thu, Oct 20, 2016 at 12:32 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> On Thu, Oct 20, 2016 at 10:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: >> >>> From: Eric Dumazet <edumazet@google.com> >> >>> >> >>> In commit 4ee3bd4a8c746 ("ipv4: disable BH when changing ip local port >> >>> range") Cong added BH protection in set_local_port_range() but missed >> >>> that same fix was needed in set_ping_group_range() >> >> >> >> Don't know why ping_group_range shares the same lock with local_port_range... >> >> Perhaps just for saving a few bytes, but that is why I missed this place. >> > >> > Hold on... We clearly have typos there... Your fix is not correct. >> >> We need the attached patch, your patch should be reverted, because >> unlike local_port_range we never read it in BH context, no need to bother _bh. > > Well, we do not change this sysctl very often, so I am not sure why we > need different seqlocks to protect these ranges. > > Seems a waste of space really (per netns) Error prone vs. space saving, it's up to you... But clearly current code is still broken even after your patch. I will send a revert + previous typo fix.
On Thu, 2016-10-20 at 14:00 -0700, Cong Wang wrote: > Error prone vs. space saving, it's up to you... > > But clearly current code is still broken even after your patch. I will send > a revert + previous typo fix. Yes please do.
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c index 1cb67de..80bc36b 100644 --- a/net/ipv4/sysctl_net_ipv4.c +++ b/net/ipv4/sysctl_net_ipv4.c @@ -96,11 +96,11 @@ static void inet_get_ping_group_range_table(struct ctl_table *table, kgid_t *low container_of(table->data, struct net, ipv4.ping_group_range.range); unsigned int seq; do { - seq = read_seqbegin(&net->ipv4.ip_local_ports.lock); + seq = read_seqbegin(&net->ipv4.ping_group_range.lock); *low = data[0]; *high = data[1]; - } while (read_seqretry(&net->ipv4.ip_local_ports.lock, seq)); + } while (read_seqretry(&net->ipv4.ping_group_range.lock, seq)); } /* Update system visible IP port range */ @@ -109,10 +109,10 @@ static void set_ping_group_range(struct ctl_table *table, kgid_t low, kgid_t hig kgid_t *data = table->data; struct net *net = container_of(table->data, struct net, ipv4.ping_group_range.range); - write_seqlock(&net->ipv4.ip_local_ports.lock); + write_seqlock(&net->ipv4.ping_group_range.lock); data[0] = low; data[1] = high; - write_sequnlock(&net->ipv4.ip_local_ports.lock); + write_sequnlock(&net->ipv4.ping_group_range.lock); } /* Validate changes from /proc interface. */