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

Commit Message

Raghavendra K T Oct. 30, 2013, 2:01 p.m. UTC

On 10/30/2013 01:03 AM, Linus Torvalds wrote:
> On Tue, Oct 29, 2013 at 12:27 PM, Raghavendra K T
> <[e-mail]> 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<---

Comments

Greg KH Oct. 30, 2013, 2:23 p.m. UTC | #1

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
> > <[e-mail]> 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 <[e-mail]>
> 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 <[e-mail]>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Raghavendra K T Oct. 30, 2013, 3:39 p.m. UTC | #2

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).

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini Oct. 30, 2013, 3:46 p.m. UTC | #3

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg KH Oct. 30, 2013, 3:59 p.m. UTC | #4

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini Oct. 30, 2013, 4:08 p.m. UTC | #5

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg KH Oct. 30, 2013, 4:23 p.m. UTC | #6

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini Oct. 30, 2013, 4:40 p.m. UTC | #7

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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

From ac5d9f038c273f27bea7a54aab6af79b57f56317 Mon Sep 17 00:00:00 2001
From: Raghavendra K T <[e-mail]>
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 <[e-mail]>
---
 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