Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Linus Torvalds]

Ugh. I won't comment on the actual kvm part of this patch, somebody
who knows that code should do so.

But I reacted to this:

On Tue, Oct 29, 2013 at 8:13 AM, Tim Gardner <tim.gardner@canonical.com> wrote:
>
> sudo modprobe kvm_amd
> modprobe: ERROR: could not insert 'kvm_amd': Bad address

"Bad address"? Christ people, are you guys making up error numbers
with some kind of dice-roll? I can just see it now, somebody sitting
there with a D20, playing some kind of kernel-specific D&D, and
rolling a ten means that you get to slay the orc, and pick an error
number of EFAULT for some random kernel function. Because quite
frankly, "random dice roll" is the _only_ thing that explains "Bad
address" sufficiently.

Please, whoever wrote virt/kvm/kvm_main.c:: kvm_init_debug(), WTF?
EFAULT means "user passed in an invalid virtual address pointer",
which is why the error string is "Bad address". It makes absolutely NO
SENSE here. Perhaps EEXIST or EBUSY.

                  Linus

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Raghavendra K T]

Adding Greg/AI too since we touch debugfs code.
[...]
 >>
>> sudo modprobe kvm_amd
>> modprobe: ERROR: could not insert 'kvm_amd': Bad address
>
> "Bad address"? Christ people, are you guys making up error numbers
> with some kind of dice-roll? I can just see it now, somebody sitting
> there with a D20, playing some kind of kernel-specific D&D, and
> rolling a ten means that you get to slay the orc, and pick an error
> number of EFAULT for some random kernel function. Because quite
> frankly, "random dice roll" is the _only_ thing that explains "Bad
> address" sufficiently.
>
> Please, whoever wrote virt/kvm/kvm_main.c:: kvm_init_debug(), WTF?
> EFAULT means "user passed in an invalid virtual address pointer",
> which is why the error string is "Bad address". It makes absolutely NO
> SENSE here. Perhaps EEXIST or EBUSY.
>

Right. In current scenario it should have been EEXIST :(.

debugfs_create_dir() currently returns NULL dentry on both
EEXIST, ENOMEM ... cases.

Could one solution be cascading actual error
that is lost in fs/debugfs/inode.c:__create_file(), so that we could
take correct action in case of failure of debugfs_create_dir()?

(ugly side is we increase total number of params for __create_file to
6). or I hope there could be some better solution.

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Linus Torvalds]

On Tue, Oct 29, 2013 at 12:27 PM, Raghavendra K T
<raghavendra.kt@linux.vnet.ibm.com> wrote:
>
> Could one solution be cascading actual error
> that is lost in fs/debugfs/inode.c:__create_file(), so that we could
> take correct action in case of failure of debugfs_create_dir()?
>
> (ugly side is we increase total number of params for __create_file to
> 6). or I hope there could be some better solution.

The solution to this would be to simply return an error-pointer. See
<linux/err.h>. That's what we do for most complex subsystems that
return a pointer to a struct: rather than returning "NULL" as an
error, return the actual error number encoded in the pointer itself.

But that would require every user of debugfs_create_dir() to be
updated to check errors using IS_ERR() instead of checking against
NULL, and there's quite a few of them.

So I think just making the error be EEXIST is a simpler solution right now.

              Linus

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Raghavendra K T]

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

On 10/30/2013 01:03 AM, Linus Torvalds wrote:
> On Tue, Oct 29, 2013 at 12:27 PM, Raghavendra K T
> <raghavendra.kt@linux.vnet.ibm.com> wrote:
>>
>> Could one solution be cascading actual error
>> that is lost in fs/debugfs/inode.c:__create_file(), so that we could
>> take correct action in case of failure of debugfs_create_dir()?
>>
>> (ugly side is we increase total number of params for __create_file to
>> 6). or I hope there could be some better solution.
>
> The solution to this would be to simply return an error-pointer. See
> <linux/err.h>. That's what we do for most complex subsystems that
> return a pointer to a struct: rather than returning "NULL" as an
> error, return the actual error number encoded in the pointer itself.

Thank you Linus. Yes, this would have been ideal.

>
> But that would require every user of debugfs_create_dir() to be
> updated to check errors using IS_ERR() instead of checking against
> NULL, and there's quite a few of them.
>
> So I think just making the error be EEXIST is a simpler solution right now.
>

Agree.
I had below patch, and just before sending as formal mail I saw
Paolo's pull request which already took care of it.
---8<---



[-- Attachment #2: 0001-Return-EEXIST-on-debugfs_create_dir-failure-in-kvm.patch --]
[-- Type: text/x-patch, Size: 1795 bytes --]

>From ac5d9f038c273f27bea7a54aab6af79b57f56317 Mon Sep 17 00:00:00 2001
From: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
Date: Wed, 30 Oct 2013 18:59:46 +0530
Subject: [PATCH] Return EEXIST on debugfs_create_dir failure in kvm

As quoted by Linus,  EFAULT means "user passed in an invalid
virtual address pointer", which is why the error string is Bad address.
But when a debugfs directory creation fails, the above error is not valid.

Signed-off-by: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
---
 I understand that Tim's patch that renames directory to something like
 kvm-pv would solve kvm-amd/kvm-intel modules insertion problem. 
 This patch is to address error code  change complained by Linus.

 arch/x86/kernel/kvm.c | 2 +-
 virt/kvm/kvm_main.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index a0e2a8a..e475fdb 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -622,7 +622,7 @@ static int __init kvm_spinlock_debugfs(void)
 
 	d_kvm = kvm_init_debugfs();
 	if (d_kvm == NULL)
-		return -ENOMEM;
+		return -EEXIST;
 
 	d_spin_debug = debugfs_create_dir("spinlocks", d_kvm);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a9dd682..0430853 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3091,13 +3091,14 @@ static const struct file_operations *stat_fops[] = {
 
 static int kvm_init_debug(void)
 {
-	int r = -EFAULT;
+	int r = -EEXIST;
 	struct kvm_stats_debugfs_item *p;
 
 	kvm_debugfs_dir = debugfs_create_dir("kvm", NULL);
 	if (kvm_debugfs_dir == NULL)
 		goto out;
 
+	r = -EFAULT;
 	for (p = debugfs_entries; p->name; ++p) {
 		p->dentry = debugfs_create_file(p->name, 0444, kvm_debugfs_dir,
 						(void *)(long)p->offset,
-- 
1.7.11.7

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Greg KH]

On Wed, Oct 30, 2013 at 07:31:05PM +0530, Raghavendra K T wrote:
> On 10/30/2013 01:03 AM, Linus Torvalds wrote:
> > On Tue, Oct 29, 2013 at 12:27 PM, Raghavendra K T
> > <raghavendra.kt@linux.vnet.ibm.com> wrote:
> >>
> >> Could one solution be cascading actual error
> >> that is lost in fs/debugfs/inode.c:__create_file(), so that we could
> >> take correct action in case of failure of debugfs_create_dir()?
> >>
> >> (ugly side is we increase total number of params for __create_file to
> >> 6). or I hope there could be some better solution.
> >
> > The solution to this would be to simply return an error-pointer. See
> > <linux/err.h>. That's what we do for most complex subsystems that
> > return a pointer to a struct: rather than returning "NULL" as an
> > error, return the actual error number encoded in the pointer itself.
> 
> Thank you Linus. Yes, this would have been ideal.
> 
> >
> > But that would require every user of debugfs_create_dir() to be
> > updated to check errors using IS_ERR() instead of checking against
> > NULL, and there's quite a few of them.
> >
> > So I think just making the error be EEXIST is a simpler solution right now.
> >
> 
> Agree.
> I had below patch, and just before sending as formal mail I saw
> Paolo's pull request which already took care of it.
> ---8<---
> 
> 

> >From ac5d9f038c273f27bea7a54aab6af79b57f56317 Mon Sep 17 00:00:00 2001
> From: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> Date: Wed, 30 Oct 2013 18:59:46 +0530
> Subject: [PATCH] Return EEXIST on debugfs_create_dir failure in kvm
> 
> As quoted by Linus,  EFAULT means "user passed in an invalid
> virtual address pointer", which is why the error string is Bad address.
> But when a debugfs directory creation fails, the above error is not valid.
> 
> Signed-off-by: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> ---
>  I understand that Tim's patch that renames directory to something like
>  kvm-pv would solve kvm-amd/kvm-intel modules insertion problem. 
>  This patch is to address error code  change complained by Linus.
> 
>  arch/x86/kernel/kvm.c | 2 +-
>  virt/kvm/kvm_main.c   | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index a0e2a8a..e475fdb 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -622,7 +622,7 @@ static int __init kvm_spinlock_debugfs(void)
>  
>  	d_kvm = kvm_init_debugfs();
>  	if (d_kvm == NULL)
> -		return -ENOMEM;
> +		return -EEXIST;

Why even error out at all here?  You should never care about debugfs
file creation return values, so why pass any error back up the stack?

greg k-h

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Raghavendra K T]

On 10/30/2013 07:53 PM, Greg KH wrote:
[...]
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index a0e2a8a..e475fdb 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -622,7 +622,7 @@ static int __init kvm_spinlock_debugfs(void)
>>
>>   	d_kvm = kvm_init_debugfs();
>>   	if (d_kvm == NULL)
>> -		return -ENOMEM;
>> +		return -EEXIST;
>
> Why even error out at all here?  You should never care about debugfs
> file creation return values, so why pass any error back up the stack?

We could change this to return 0, but we will still be left with
kvm_main.c: kvm_init_debug() function which creates the kvm  debugfs
directory. (I thought Paolo and Gleb's ack would be needed for
that change since it would be a bigger decision for me)

So I just made the patch to fix only Linus's concern.

But sorry that I did not make that explicit. (infact module insertion 
error was because of successful creation of kvm directory in above code
and then error coming from kvm_init_debug() in kvm_main.c).

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Paolo Bonzini]

Il 30/10/2013 16:39, Raghavendra K T ha scritto:
>>
>> Why even error out at all here?  You should never care about debugfs
>> file creation return values, so why pass any error back up the stack?
> 
> We could change this to return 0, but we will still be left with
> kvm_main.c: kvm_init_debug() function which creates the kvm  debugfs
> directory. (I thought Paolo and Gleb's ack would be needed for
> that change since it would be a bigger decision for me)
> 
> So I just made the patch to fix only Linus's concern.

Even if it is okay to exit and not create the files (and I think it's a
bit surprising), I'd like at least a printk to signal what's happening.
 But there should be no reason for debugfs directory creation to fail in
the end, except for basic mistakes such as the one that Tim reported, so
I think it's better to report the failure.

> But sorry that I did not make that explicit. (infact module insertion
> error was because of successful creation of kvm directory in above code
> and then error coming from kvm_init_debug() in kvm_main.c).

Yes, and I'm keeping the error reporting there too, while fixing the
bogus EFAULT.

Paolo

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Greg KH]

On Wed, Oct 30, 2013 at 04:46:28PM +0100, Paolo Bonzini wrote:
> Il 30/10/2013 16:39, Raghavendra K T ha scritto:
> >>
> >> Why even error out at all here?  You should never care about debugfs
> >> file creation return values, so why pass any error back up the stack?
> > 
> > We could change this to return 0, but we will still be left with
> > kvm_main.c: kvm_init_debug() function which creates the kvm  debugfs
> > directory. (I thought Paolo and Gleb's ack would be needed for
> > that change since it would be a bigger decision for me)
> > 
> > So I just made the patch to fix only Linus's concern.
> 
> Even if it is okay to exit and not create the files (and I think it's a
> bit surprising), I'd like at least a printk to signal what's happening.
>  But there should be no reason for debugfs directory creation to fail in
> the end, except for basic mistakes such as the one that Tim reported, so
> I think it's better to report the failure.

Creation will "fail" if debugfs is not enabled, so spiting out printk
messages in that case is not a good idea.

greg k-h

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Paolo Bonzini]

Il 30/10/2013 16:59, Greg KH ha scritto:
>> > Even if it is okay to exit and not create the files (and I think it's a
>> > bit surprising), I'd like at least a printk to signal what's happening.
>> >  But there should be no reason for debugfs directory creation to fail in
>> > the end, except for basic mistakes such as the one that Tim reported, so
>> > I think it's better to report the failure.
> Creation will "fail" if debugfs is not enabled, so spiting out printk
> messages in that case is not a good idea.

Interestingly, if debugfs is not enabled we are already returning an
error-valued pointer:

static inline struct dentry *debugfs_create_dir(const char *name,
                                                struct dentry *parent)
{
        return ERR_PTR(-ENODEV);
}

which would oops a lot of the current callers.  Very few places use the
currently correct idiom

	if (IS_ERR(root) || !root)

but it's very ugly...  Perhaps debugfs_create_dir *should* return an
error-valued pointer after all.

Paolo

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Greg KH]

On Wed, Oct 30, 2013 at 05:08:09PM +0100, Paolo Bonzini wrote:
> Il 30/10/2013 16:59, Greg KH ha scritto:
> >> > Even if it is okay to exit and not create the files (and I think it's a
> >> > bit surprising), I'd like at least a printk to signal what's happening.
> >> >  But there should be no reason for debugfs directory creation to fail in
> >> > the end, except for basic mistakes such as the one that Tim reported, so
> >> > I think it's better to report the failure.
> > Creation will "fail" if debugfs is not enabled, so spiting out printk
> > messages in that case is not a good idea.
> 
> Interestingly, if debugfs is not enabled we are already returning an
> error-valued pointer:
> 
> static inline struct dentry *debugfs_create_dir(const char *name,
>                                                 struct dentry *parent)
> {
>         return ERR_PTR(-ENODEV);
> }
> 
> which would oops a lot of the current callers.

It will oops?  Really?  Where?  That shouldn't happen at all.

> Very few places use the currently correct idiom
> 
> 	if (IS_ERR(root) || !root)
> 
> but it's very ugly...  Perhaps debugfs_create_dir *should* return an
> error-valued pointer after all.

Or just don't care about the return value, and all will work out just
fine, right?

thanks,

greg k-h

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Paolo Bonzini]

Il 30/10/2013 17:23, Greg KH ha scritto:
>> > static inline struct dentry *debugfs_create_dir(const char *name,
>> >                                                 struct dentry *parent)
>> > {
>> >         return ERR_PTR(-ENODEV);
>> > }
>> > 
>> > which would oops a lot of the current callers.
> It will oops?  Really?  Where?  That shouldn't happen at all.

Doh, it obviously won't because the bogus pointer value will never be
dereferenced by the dummy debugfs_create_file.

>> > Very few places use the currently correct idiom
>> > 
>> > 	if (IS_ERR(root) || !root)
>> > 
>> > but it's very ugly...  Perhaps debugfs_create_dir *should* return an
>> > error-valued pointer after all.
> Or just don't care about the return value, and all will work out just
> fine, right?

Debugfs files would then be created in the debugfs root, which is not nice.

        /* If the parent is not specified, we create it in the root.
         * We need the root dentry to do this, which is in the super 
         * block. A pointer to that is in the struct vfsmount that we
         * have around.
         */
        if (!parent)
                parent = debugfs_mount->mnt_root;

This is all documented right:

/**
 * debugfs_create_file - create a file in the debugfs filesystem
 * ...
 * This function will return a pointer to a dentry if it succeeds.  This
 * pointer must be passed to the debugfs_remove() function when the file is
 * to be removed (no automatic cleanup happens if your module is unloaded,
 * you are responsible here.)  If an error occurs, %NULL will be returned.
 *
 * If debugfs is not enabled in the kernel, the value -%ENODEV will be
 * returned.
 */

so I guess it's just part of the API.

Paolo

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Greg KH]

On Wed, Oct 30, 2013 at 12:57:32AM +0530, Raghavendra K T wrote:
> Adding Greg/AI too since we touch debugfs code.

You do?

> [...]
> >>
> >>sudo modprobe kvm_amd
> >>modprobe: ERROR: could not insert 'kvm_amd': Bad address
> >
> >"Bad address"? Christ people, are you guys making up error numbers
> >with some kind of dice-roll? I can just see it now, somebody sitting
> >there with a D20, playing some kind of kernel-specific D&D, and
> >rolling a ten means that you get to slay the orc, and pick an error
> >number of EFAULT for some random kernel function. Because quite
> >frankly, "random dice roll" is the _only_ thing that explains "Bad
> >address" sufficiently.
> >
> >Please, whoever wrote virt/kvm/kvm_main.c:: kvm_init_debug(), WTF?
> >EFAULT means "user passed in an invalid virtual address pointer",
> >which is why the error string is "Bad address". It makes absolutely NO
> >SENSE here. Perhaps EEXIST or EBUSY.
> >
> 
> Right. In current scenario it should have been EEXIST :(.
> 
> debugfs_create_dir() currently returns NULL dentry on both
> EEXIST, ENOMEM ... cases.
> 
> Could one solution be cascading actual error
> that is lost in fs/debugfs/inode.c:__create_file(), so that we could
> take correct action in case of failure of debugfs_create_dir()?

What would you do here?  You shouldn't really care about debugfs files,
if there's an error, keep on going, no code path should really care,
right?

thanks,

greg k-h

Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Raghavendra K T]

On 10/30/2013 01:30 AM, Greg KH wrote:
[...]
>> debugfs_create_dir() currently returns NULL dentry on both
>> EEXIST, ENOMEM ... cases.
>>
>> Could one solution be cascading actual error
>> that is lost in fs/debugfs/inode.c:__create_file(), so that we could
>> take correct action in case of failure of debugfs_create_dir()?
>
> What would you do here?  You shouldn't really care about debugfs files,
> if there's an error, keep on going, no code path should really care,
> right?
>
Thanks Greg.

Yes you are right. we can't do anything useful after that error.

If debugfs directories are not critical, may be we could
continue from there.