[bug report] chardev: code cleanup for __register_chrdev_region()

* [bug report] chardev: code cleanup for __register_chrdev_region()
@ 2019-04-04 19:08 Dan Carpenter
  2019-04-04 23:13 ` cgxu519
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2019-04-04 19:08 UTC (permalink / raw)
  To: cgxu519; +Cc: linux-fsdevel

Hello Chengguang Xu,

The patch 4b0be5726032: "chardev: code cleanup for
__register_chrdev_region()" from Feb 15, 2019, leads to the following
static checker warning:

	fs/char_dev.c:167 __register_chrdev_region()
	error: passing non negative 511 to ERR_PTR

fs/char_dev.c
    96  static struct char_device_struct *
    97  __register_chrdev_region(unsigned int major, unsigned int baseminor,
    98                             int minorct, const char *name)
    99  {
   100          struct char_device_struct *cd, *curr, *prev = NULL;
   101          int ret = -EBUSY;
   102          int i;
   103  
   104          if (major >= CHRDEV_MAJOR_MAX) {
   105                  pr_err("CHRDEV \"%s\" major requested (%u) is greater than the maximum (%u)\n",
   106                         name, major, CHRDEV_MAJOR_MAX-1);
   107                  return ERR_PTR(-EINVAL);
   108          }
   109  
   110          if (minorct > MINORMASK + 1 - baseminor) {
   111                  pr_err("CHRDEV \"%s\" minor range requested (%u-%u) is out of range of maximum range (%u-%u) for a single major\n",
   112                          name, baseminor, baseminor + minorct - 1, 0, MINORMASK);
   113                  return ERR_PTR(-EINVAL);
   114          }
   115  
   116          cd = kzalloc(sizeof(struct char_device_struct), GFP_KERNEL);
   117          if (cd == NULL)
   118                  return ERR_PTR(-ENOMEM);
   119  
   120          mutex_lock(&chrdevs_lock);
   121  
   122          if (major == 0) {
   123                  ret = find_dynamic_major();
   124                  if (ret < 0) {
   125                          pr_err("CHRDEV \"%s\" dynamic allocation region is full\n",
   126                                 name);
   127                          goto out;
   128                  }
   129                  major = ret;
                        ^^^^^^^^^^^
"ret" is a major here.

   130          }
   131  
   132          i = major_to_index(major);
   133          for (curr = chrdevs[i]; curr; prev = curr, curr = curr->next) {
   134                  if (curr->major < major)
   135                          continue;
   136  
   137                  if (curr->major > major)
   138                          break;
   139  
   140                  if (curr->baseminor + curr->minorct <= baseminor)
   141                          continue;
   142  
   143                  if (curr->baseminor >= baseminor + minorct)
   144                          break;
   145  
   146                  goto out;

I don't completely understand how this loop works, but I guess we should
set "ret = -EBUSY;" before the goto out.

   147          }
   148  
   149          cd->major = major;
   150          cd->baseminor = baseminor;
   151          cd->minorct = minorct;
   152          strlcpy(cd->name, name, sizeof(cd->name));
   153  
   154          if (!prev) {
   155                  cd->next = curr;
   156                  chrdevs[i] = cd;
   157          } else {
   158                  cd->next = prev->next;
   159                  prev->next = cd;
   160          }
   161  
   162          mutex_unlock(&chrdevs_lock);
   163          return cd;
   164  out:
   165          mutex_unlock(&chrdevs_lock);
   166          kfree(cd);
   167          return ERR_PTR(ret);
                               ^^^
Otherwise it leads to an Oops in the caller.

   168  }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 4+ messages in thread