[PATCH 20/21] phonet: exit_net cleanup check added

[PATCH 20/21] phonet: exit_net cleanup check added

[Vasily Averin]

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 = {
-- 
2.7.4

Re: [PATCH 20/21] phonet: exit_net cleanup check added

[Rémi Denis-Courmont]

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?
-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.

Re: [PATCH 20/21] phonet: exit_net cleanup check added

[Vasily Averin]

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

Re: [PATCH 20/21] phonet: exit_net cleanup check added

[David Miller]

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.

Re: [PATCH 20/21] phonet: exit_net cleanup check added

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24640 bytes --]

How to identify net namespace in kernel messages?

[Vasily Averin]

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?

Re: How to identify net namespace in kernel messages?

[David Ahern]

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.