| Message ID | 3dcdb5aa-cb48-dcf7-f8f4-80e9c36810aa@virtuozzo.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | exit_net checks for objects initialized in net_init hook | expand |
Le 5 novembre 2017 12:02:34 GMT+02:00, Vasily Averin <vvs@virtuozzo.com> a écrit : >Be sure that pndevs.list initialized in net_init hook was return >to initial state. > >Signed-off-by: Vasily Averin <vvs@virtuozzo.com> >--- > net/phonet/pn_dev.c | 2 ++ > 1 file changed, 2 insertions(+) > >diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c >index 2cb4c5d..1024851 100644 >--- a/net/phonet/pn_dev.c >+++ b/net/phonet/pn_dev.c >@@ -332,6 +332,8 @@ static int __net_init phonet_init_net(struct net >*net) > static void __net_exit phonet_exit_net(struct net *net) > { > remove_proc_entry("phonet", net->proc_net); >+ WARN(!list_empty(&pnn->pndevs.list), >+ "net %p exit: phonet pndevs.list is not empty\n", net); > } > > static struct pernet_operations phonet_net_ops = { Hello, TBH, I am not clear what the benefit of this is supposed to be... also, does this not leak a pointer to userspace, breaking ASLR?
On 2017-11-05 13:17, Rémi Denis-Courmont wrote: > Le 5 novembre 2017 12:02:34 GMT+02:00, Vasily Averin <vvs@virtuozzo.com> a écrit : >> Be sure that pndevs.list initialized in net_init hook was return >> to initial state. >> >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> >> --- >> net/phonet/pn_dev.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c >> index 2cb4c5d..1024851 100644 >> --- a/net/phonet/pn_dev.c >> +++ b/net/phonet/pn_dev.c >> @@ -332,6 +332,8 @@ static int __net_init phonet_init_net(struct net >> *net) >> static void __net_exit phonet_exit_net(struct net *net) >> { >> remove_proc_entry("phonet", net->proc_net); >> + WARN(!list_empty(&pnn->pndevs.list), >> + "net %p exit: phonet pndevs.list is not empty\n", net); >> } >> >> static struct pernet_operations phonet_net_ops = { > > Hello, > > TBH, I am not clear what the benefit of this is supposed to be... also, does this not leak a pointer to userspace, breaking ASLR? Dear Rémi, I assume that elements added into per-netns list should not live longer then netns. Such check allows to be sure that driver works with list correctly and lost no entries. Many drivers does it already, and my current patch sent just makes this praxis global. I doubt that pointer to freed net have value for someone except developers, on the other hand it helps to speed up the problem investigation. Thank you, Vasily Averin
From: Vasily Averin <vvs@virtuozzo.com> Date: Sun, 5 Nov 2017 13:52:25 +0300 > I doubt that pointer to freed net have value for someone except > developers, on the other hand it helps to speed up the problem > investigation. Any kernel pointer printed has value to attackers.
Hi Vasily,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on v4.14-rc7 next-20171103]
[cannot apply to net-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Vasily-Averin/exit_net-checks-for-objects-initialized-in-net_init-hook/20171105-201757
config: x86_64-randconfig-x018-201745 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from arch/x86/include/asm/bug.h:82:0,
from include/linux/bug.h:5,
from include/linux/jump_label.h:187,
from include/linux/once.h:6,
from include/linux/random.h:11,
from include/linux/net.h:22,
from net/phonet/pn_dev.c:27:
net/phonet/pn_dev.c: In function 'phonet_exit_net':
>> net/phonet/pn_dev.c:335:20: error: 'pnn' undeclared (first use in this function)
WARN(!list_empty(&pnn->pndevs.list),
^
include/asm-generic/bug.h:117:25: note: in definition of macro 'WARN'
int __ret_warn_on = !!(condition); \
^~~~~~~~~
net/phonet/pn_dev.c:335:20: note: each undeclared identifier is reported only once for each function it appears in
WARN(!list_empty(&pnn->pndevs.list),
^
include/asm-generic/bug.h:117:25: note: in definition of macro 'WARN'
int __ret_warn_on = !!(condition); \
^~~~~~~~~
vim +/pnn +335 net/phonet/pn_dev.c
331
332 static void __net_exit phonet_exit_net(struct net *net)
333 {
334 remove_proc_entry("phonet", net->proc_net);
> 335 WARN(!list_empty(&pnn->pndevs.list),
336 "net %p exit: phonet pndevs.list is not empty\n", net);
337 }
338
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 2017-11-05 15:48, David Miller wrote: > From: Vasily Averin <vvs@virtuozzo.com> >> I doubt that pointer to freed net have value for someone except >> developers, on the other hand it helps to speed up the problem >> investigation. > > Any kernel pointer printed has value to attackers. David, could you please advise how to identify net namespace in kernel messages? In OpenVz we got many requests from host admins, they need to understand which container triggered the message. In such cases we have added our custom Container Id, but mainline lacks it. I expected that mainline can use net pointer for such purposes, nfsd does it for example: NFSD: starting 90-second grace period (net ffff880e307fe240) Now you recommend do not use net pointer. However could you please advise some alternative?
On 11/6/17 5:56 AM, Vasily Averin wrote: > On 2017-11-05 15:48, David Miller wrote: >> From: Vasily Averin <vvs@virtuozzo.com> >>> I doubt that pointer to freed net have value for someone except >>> developers, on the other hand it helps to speed up the problem >>> investigation. >> >> Any kernel pointer printed has value to attackers. > > David, could you please advise how to identify net namespace in kernel messages? > > In OpenVz we got many requests from host admins, they need to understand > which container triggered the message. In such cases we have added our custom > Container Id, but mainline lacks it. > > I expected that mainline can use net pointer for such purposes, > nfsd does it for example: > > NFSD: starting 90-second grace period (net ffff880e307fe240) > > Now you recommend do not use net pointer. > However could you please advise some alternative? > Perf now exports the device and inode. see perf_ns_link_info and its use.
diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c index 2cb4c5d..1024851 100644 --- a/net/phonet/pn_dev.c +++ b/net/phonet/pn_dev.c @@ -332,6 +332,8 @@ static int __net_init phonet_init_net(struct net *net) static void __net_exit phonet_exit_net(struct net *net) { remove_proc_entry("phonet", net->proc_net); + WARN(!list_empty(&pnn->pndevs.list), + "net %p exit: phonet pndevs.list is not empty\n", net); } static struct pernet_operations phonet_net_ops = {
Be sure that pndevs.list initialized in net_init hook was return to initial state. Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- net/phonet/pn_dev.c | 2 ++ 1 file changed, 2 insertions(+)