* [PATCH net] ipv4: disable BH in set_ping_group_range()
@ 2016-10-20 17:26 Eric Dumazet
2016-10-20 18:50 ` David Miller
2016-10-20 19:32 ` Cong Wang
0 siblings, 2 replies; 8+ messages in thread
From: Eric Dumazet @ 2016-10-20 17:26 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Eric Salo, Cong Wang
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()
Fixes: b8f1a55639e6 ("udp: Add function to make source port for UDP tunnels")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Eric Salo <salo@google.com>
---
net/ipv4/sysctl_net_ipv4.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 1cb67de106fe..500ae4010bed 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -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_bh(&net->ipv4.ip_local_ports.lock);
data[0] = low;
data[1] = high;
- write_sequnlock(&net->ipv4.ip_local_ports.lock);
+ write_sequnlock_bh(&net->ipv4.ip_local_ports.lock);
}
/* Validate changes from /proc interface. */
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net] ipv4: disable BH in set_ping_group_range() 2016-10-20 17:26 [PATCH net] ipv4: disable BH in set_ping_group_range() Eric Dumazet @ 2016-10-20 18:50 ` David Miller 2016-10-20 19:32 ` Cong Wang 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2016-10-20 18:50 UTC (permalink / raw) To: eric.dumazet; +Cc: netdev, salo, xiyou.wangcong From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 20 Oct 2016 10:26:48 -0700 > 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() > > Fixes: b8f1a55639e6 ("udp: Add function to make source port for UDP tunnels") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Eric Salo <salo@google.com> Applied and queued up for -stable. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range() 2016-10-20 17:26 [PATCH net] ipv4: disable BH in set_ping_group_range() Eric Dumazet 2016-10-20 18:50 ` David Miller @ 2016-10-20 19:32 ` Cong Wang 2016-10-20 19:40 ` Cong Wang 1 sibling, 1 reply; 8+ messages in thread From: Cong Wang @ 2016-10-20 19:32 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range() 2016-10-20 19:32 ` Cong Wang @ 2016-10-20 19:40 ` Cong Wang 2016-10-20 19:44 ` Cong Wang 0 siblings, 1 reply; 8+ messages in thread From: Cong Wang @ 2016-10-20 19:40 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range() 2016-10-20 19:40 ` Cong Wang @ 2016-10-20 19:44 ` Cong Wang 2016-10-20 20:43 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Cong Wang @ 2016-10-20 19:44 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo [-- Attachment #1: Type: text/plain, Size: 872 bytes --] 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. [-- Attachment #2: ping.diff --] [-- Type: text/plain, Size: 1211 bytes --] 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. */ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range() 2016-10-20 19:44 ` Cong Wang @ 2016-10-20 20:43 ` Eric Dumazet 2016-10-20 21:00 ` Cong Wang 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2016-10-20 20:43 UTC (permalink / raw) To: Cong Wang; +Cc: David Miller, netdev, Eric Salo 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range() 2016-10-20 20:43 ` Eric Dumazet @ 2016-10-20 21:00 ` Cong Wang 2016-10-20 21:08 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Cong Wang @ 2016-10-20 21:00 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range() 2016-10-20 21:00 ` Cong Wang @ 2016-10-20 21:08 ` Eric Dumazet 0 siblings, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2016-10-20 21:08 UTC (permalink / raw) To: Cong Wang; +Cc: David Miller, netdev, Eric Salo 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-10-20 21:08 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-20 17:26 [PATCH net] ipv4: disable BH in set_ping_group_range() Eric Dumazet 2016-10-20 18:50 ` David Miller 2016-10-20 19:32 ` Cong Wang 2016-10-20 19:40 ` Cong Wang 2016-10-20 19:44 ` Cong Wang 2016-10-20 20:43 ` Eric Dumazet 2016-10-20 21:00 ` Cong Wang 2016-10-20 21:08 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).