| Message ID | 1474542209-28409-1-git-send-email-hannes@stressinduktion.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2016-09-22 at 13:03 +0200, Hannes Frederic Sowa wrote: > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > net/core/net_namespace.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c > index 2c2eb1b629b11d..a2ace299f28355 100644 > --- a/net/core/net_namespace.c > +++ b/net/core/net_namespace.c > @@ -758,9 +758,11 @@ static int __init net_ns_init(void) > > register_pernet_subsys(&net_ns_ops); > > + rtnl_lock(); > rtnl_register(PF_UNSPEC, RTM_NEWNSID, rtnl_net_newid, NULL, NULL); > rtnl_register(PF_UNSPEC, RTM_GETNSID, rtnl_net_getid, rtnl_net_dumpid, > NULL); > + rtnl_unlock(); > > return 0; > } Hi Hannes Why is this needed here, and not in other places ? Hint : A changelog always help reviewers and future bug hunting. Thanks.
On 22.09.2016 15:03, Eric Dumazet wrote: > On Thu, 2016-09-22 at 13:03 +0200, Hannes Frederic Sowa wrote: >> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> >> --- >> net/core/net_namespace.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c >> index 2c2eb1b629b11d..a2ace299f28355 100644 >> --- a/net/core/net_namespace.c >> +++ b/net/core/net_namespace.c >> @@ -758,9 +758,11 @@ static int __init net_ns_init(void) >> >> register_pernet_subsys(&net_ns_ops); >> >> + rtnl_lock(); >> rtnl_register(PF_UNSPEC, RTM_NEWNSID, rtnl_net_newid, NULL, NULL); >> rtnl_register(PF_UNSPEC, RTM_GETNSID, rtnl_net_getid, rtnl_net_dumpid, >> NULL); >> + rtnl_unlock(); >> >> return 0; >> } > > Hi Hannes > > Why is this needed here, and not in other places ? I found this during working on the file and actually saw no live issues (belonged to another series which I just split up). I don't think it is a big issue but wanted the writes to the rtnl_msg_handlers array to be strictly serialized. I was working on adding this to other places, too. Maybe better for net-next even? Theoretically we would need to add a memory barriers to make sure we don't publish uninitialized memory into the array if concurrent readers of the array want to find their function pointers. > Hint : A changelog always help reviewers and future bug hunting. I will add that to v2. Thanks, Hannes
On Thu, 2016-09-22 at 15:41 +0200, Hannes Frederic Sowa wrote: > I found this during working on the file and actually saw no live issues > (belonged to another series which I just split up). > > I don't think it is a big issue but wanted the writes to the > rtnl_msg_handlers array to be strictly serialized. I was working on > adding this to other places, too. Maybe better for net-next even? Sure, and you probably could enforce an ASSERT_RTNL() in rtnl_register() to catch all offenders.
On Thu, Sep 22, 2016 at 6:41 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On 22.09.2016 15:03, Eric Dumazet wrote: >> On Thu, 2016-09-22 at 13:03 +0200, Hannes Frederic Sowa wrote: >>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> >>> --- >>> net/core/net_namespace.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c >>> index 2c2eb1b629b11d..a2ace299f28355 100644 >>> --- a/net/core/net_namespace.c >>> +++ b/net/core/net_namespace.c >>> @@ -758,9 +758,11 @@ static int __init net_ns_init(void) >>> >>> register_pernet_subsys(&net_ns_ops); >>> >>> + rtnl_lock(); >>> rtnl_register(PF_UNSPEC, RTM_NEWNSID, rtnl_net_newid, NULL, NULL); >>> rtnl_register(PF_UNSPEC, RTM_GETNSID, rtnl_net_getid, rtnl_net_dumpid, >>> NULL); >>> + rtnl_unlock(); >>> >>> return 0; >>> } >> >> Hi Hannes >> >> Why is this needed here, and not in other places ? > > I found this during working on the file and actually saw no live issues > (belonged to another series which I just split up). > > I don't think it is a big issue but wanted the writes to the > rtnl_msg_handlers array to be strictly serialized. I was working on > adding this to other places, too. Maybe better for net-next even? But they are called during boot, why is it possible to have a parallel reader/writer at that time?
On Thu, Sep 22, 2016, at 19:20, Cong Wang wrote: > > I don't think it is a big issue but wanted the writes to the > > rtnl_msg_handlers array to be strictly serialized. I was working on > > adding this to other places, too. Maybe better for net-next even? > > But they are called during boot, why is it possible to have a parallel > reader/writer at that time? It also happens during module load time, which isn't synchronized with rtnl_lock. Bye, Hannes
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c index 2c2eb1b629b11d..a2ace299f28355 100644 --- a/net/core/net_namespace.c +++ b/net/core/net_namespace.c @@ -758,9 +758,11 @@ static int __init net_ns_init(void) register_pernet_subsys(&net_ns_ops); + rtnl_lock(); rtnl_register(PF_UNSPEC, RTM_NEWNSID, rtnl_net_newid, NULL, NULL); rtnl_register(PF_UNSPEC, RTM_GETNSID, rtnl_net_getid, rtnl_net_dumpid, NULL); + rtnl_unlock(); return 0; }
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> --- net/core/net_namespace.c | 2 ++ 1 file changed, 2 insertions(+)