Message-ID: <CALkUMdTFaUOurzkRT5y8SB07sdPQ0PvAutcpNhE-W2ZiyzTbxw@mail.gmail.com>
Date: Mon, 14 Oct 2024 12:09:37 +0530
From: Vimal Agrawal <avimalin@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>, 
	D Scott Phillips <scott@...amperecomputing.com>, Vimal Agrawal <vimal.agrawal@...hos.com>
Subject: Re: misc_deregister() throwing warning in ida_free()

Hi Greg,

I checked the return value of misc_regiser() and it is passing.
Following is the complete code of call to misc_register():
<<
static struct miscdevice ust_dev = {
        .minor  = 0,
        .name   = "ustdev",
        .fops   = &ustdev_ops,
};

int ustdev_init(void)
{
        int result = 0;

        result = misc_register(&ust_dev);
        if (result < 0) {
                KLOGERR("%s: misc_register for ustdev failed\n", __func__);
                return result;
        }
...
}
>>

There are two cases that are supported for minor numbers for misc
devices. One is using MISC_DYNAMIC_MINOR where api itself allocates a
minor number for it and updates it. Other is where the caller provides
the minor number which may fall in the range of (0, DYNAMIC_MINORS) or
> MISC_DYNAMIC_MINOR and my case is the later one as I am passing
minor as 0. To support both, we may have to allocate id in both cases
so that allocated id is seen the same in both cases.

I tried the following change which seems to be working but I am not
very sure if this is the best way to fix it:

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 541edc26ec89..d7933fabb8d8 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -63,6 +63,17 @@ static DEFINE_MUTEX(misc_mtx);
 #define DYNAMIC_MINORS 128 /* like dynamic majors */
 static DEFINE_IDA(misc_minors_ida);

+/* allocate a specific minor */
+static int misc_minor_alloc_static(int minor)
+{
+       int ret;
+
+       if (minor < DYNAMIC_MINORS)
+               minor = DYNAMIC_MINORS - minor - 1;
+
+       ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+       return ret;
+}
 static int misc_minor_alloc(void)
 {
        int ret;
@@ -228,6 +239,7 @@ int misc_register(struct miscdevice *misc)
                misc->minor = i;
        } else {
                struct miscdevice *c;
+               int i;

                list_for_each_entry(c, &misc_list, list) {
                        if (c->minor == misc->minor) {
@@ -235,6 +247,12 @@ int misc_register(struct miscdevice *misc)
                                goto out;
                        }
                }
+
+               i = misc_minor_alloc_static(misc->minor);
+               if (i < 0) {
+                       err = -EBUSY;
+                       goto out;
+               }
        }

        dev = MKDEV(MISC_MAJOR, misc->minor);

Will send the patch shortly.


Thanks,
Vimal

On Mon, Oct 7, 2024 at 1:58 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Mon, Sep 16, 2024 at 06:56:18PM +0530, Vimal Agrawal wrote:
> > Hi Scott/ Greg and all,
> >
> > We recently upgraded kernel from 6.1 to 6.6.49 and started seeing
> > following WARNING during misc_deregister():
> >
> > ------------[ cut here ]------------
> >
> > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > ida_free called for id=127 which is not allocated.
> > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> > Modules linked in: ust(O-) [last unloaded: fastpath_dummy(O)]
> > CPU: 0 PID: 159 Comm: modprobe Tainted: G        W  O     N 6.6.49+ #11
> > Stack:
> > 818bfb70 6093f8d3 0000020d 61381660
> > 61381630 60c1ee6a 00000001 60068c0f
> > 818bfbb0 60983ee6 60983e5c 61381680
> > Call Trace:
> > [<60973831>] ? _printk+0x0/0xc2
> > [<6004b2e6>] show_stack+0x35c/0x382
> > [<6093f8d3>] ? dump_stack_print_info+0x1af/0x1ec
> > [<60068c0f>] ? um_set_signals+0x0/0x43
> > [<60983ee6>] dump_stack_lvl+0x8a/0xa9
> > [<60983e5c>] ? dump_stack_lvl+0x0/0xa9
> > [<60068c0f>] ? um_set_signals+0x0/0x43
> > [<60983f32>] dump_stack+0x2d/0x35
> > [<60983f05>] ? dump_stack+0x0/0x35
> > [<6007aad8>] __warn+0x20c/0x294
> > [<60068c0f>] ? um_set_signals+0x0/0x43
> > [<60971d98>] warn_slowpath_fmt+0x164/0x189
> > [<60222128>] ? __update_cpu_freelist_fast+0x96/0xbc
> > [<60971c34>] ? warn_slowpath_fmt+0x0/0x189
> > [<6022d2fe>] ? __kmem_cache_free+0x16a/0x1be
> > [<60068c4a>] ? um_set_signals+0x3b/0x43
> > [<60941eb4>] ida_free+0x3e0/0x41f
> > [<605ac993>] misc_minor_free+0x3e/0xbc
> > [<605acb82>] misc_deregister+0x171/0x1b3
> > [<81aa7af2>] ustdev_exit+0xa8/0xc1 [ust]
> >
> > basic code of calling misc_register()/misc_register() is following:
> >
> > static struct miscdevice ust_dev = {
> >         0,
> >         "ustdev",
> >         &ustdev_ops,
> > };
>
> Nit, use named-identifiers pleaase so we know what is being set and what
> isn't.
>
> > int ustdev_init(void)
> > {
> >         misc_register(&ust_dev);
> >         return 0;
>
> This can be just one line, you are ignoring if misc_register() fails :(
>
> > }
> >
> > void ustdev_exit(void)
> > {
> >         misc_deregister(&ust_dev);
> > }
> >
> > Note that this was working fine without any warning earlier on kernel 6.1.
> >
> > I suspect it is due to
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v6.6.51&id=ab760791c0cfbb1d7a668f46a135264f56c8f018.
> > It seems misc_register() is not calling any ida_allocxxx() api for
> > static minor value of 0 but misc_deregister() for the same is calling
> > ida_free() and hence ida_free() is warning in our case.
> >
> > Please let me know if I am missing something or some of our
> > assumptions are not valid anymore in newer kernel versions.
>
> Can you submit a patch that works for you to resolve this issue?
>
> thanks,
>
> greg k-h

Powered by blists - more mailing lists