[bug report] ocxl: Add AFU interrupt support

3 messages, 2 authors, 2018-02-14 · open the first message on its own page

[bug report] ocxl: Add AFU interrupt support

From: Dan Carpenter <hidden>
Date: 2018-02-13 08:12:48

Hello Frederic Barrat,

The patch aeddad1760ae: "ocxl: Add AFU interrupt support" from Jan
23, 2018, leads to the following static checker warning:

    drivers/misc/ocxl/file.c:163 afu_ioctl()
    warn: maybe return -EFAULT instead of the bytes remaining?

drivers/misc/ocxl/file.c
   111  static long afu_ioctl(struct file *file, unsigned int cmd,
   112                  unsigned long args)
   113  {
   114          struct ocxl_context *ctx = file->private_data;
   115          struct ocxl_ioctl_irq_fd irq_fd;
   116          u64 irq_offset;
   117          long rc;
   118  
   119          pr_debug("%s for context %d, command %s\n", __func__, ctx->pasid,
   120                  CMD_STR(cmd));
   121  
   122          if (ctx->status == CLOSED)
   123                  return -EIO;
   124  
   125          switch (cmd) {
   126          case OCXL_IOCTL_ATTACH:
   127                  rc = afu_ioctl_attach(ctx,
   128                                  (struct ocxl_ioctl_attach __user *) args);
   129                  break;
   130  
   131          case OCXL_IOCTL_IRQ_ALLOC:
   132                  rc = ocxl_afu_irq_alloc(ctx, &irq_offset);
   133                  if (!rc) {
   134                          rc = copy_to_user((u64 __user *) args, &irq_offset,
   135                                          sizeof(irq_offset));
   136                          if (rc)
                                    ^^
copy_to_user() returns the number of bytes remaining but we want to
return -EFAULT on error.

   137                                  ocxl_afu_irq_free(ctx, irq_offset);
   138                  }
   139                  break;
   140  

    drivers/misc/ocxl/file.c:320 afu_read()
    warn: unsigned 'used' is never less than zero.

drivers/misc/ocxl/file.c
   279          ssize_t rc;
   280          size_t used = 0;
                ^^^^^^
This should be ssize_t

   281          DEFINE_WAIT(event_wait);
   282  
   283          memset(&header, 0, sizeof(header));
   284  
   285          /* Require offset to be 0 */
   286          if (*off != 0)
   287                  return -EINVAL;
   288  
   289          if (count < (sizeof(struct ocxl_kernel_event_header) +
   290                          AFU_EVENT_BODY_MAX_SIZE))
   291                  return -EINVAL;
   292  
   293          for (;;) {
   294                  prepare_to_wait(&ctx->events_wq, &event_wait,
   295                                  TASK_INTERRUPTIBLE);
   296  
   297                  if (afu_events_pending(ctx))
   298                          break;
   299  
   300                  if (ctx->status == CLOSED)
   301                          break;
   302  
   303                  if (file->f_flags & O_NONBLOCK) {
   304                          finish_wait(&ctx->events_wq, &event_wait);
   305                          return -EAGAIN;
   306                  }
   307  
   308                  if (signal_pending(current)) {
   309                          finish_wait(&ctx->events_wq, &event_wait);
   310                          return -ERESTARTSYS;
   311                  }
   312  
   313                  schedule();
   314          }
   315  
   316          finish_wait(&ctx->events_wq, &event_wait);
   317  
   318          if (has_xsl_error(ctx)) {
   319                  used = append_xsl_error(ctx, &header, buf + sizeof(header));
   320                  if (used < 0)
                            ^^^^^^^^
Impossible.

   321                          return used;
   322          }
   323  
   324          if (!afu_events_pending(ctx))
   325                  header.flags |= OCXL_KERNEL_EVENT_FLAG_LAST;
   326  
   327          if (copy_to_user(buf, &header, sizeof(header)))
   328                  return -EFAULT;
   329  
   330          used += sizeof(header);
   331  
   332          rc = (ssize_t) used;
                     ^^^^^^^^^^^^^^
You could remove the cast.

   333          return rc;
   334  }

regards,
dan carpenter

Re: [bug report] ocxl: Add AFU interrupt support

From: Frederic Barrat <hidden>
Date: 2018-02-13 19:29:34

Hi,

Thanks for the report. I'll fix the first issue. The 2nd is already on 
its way to upstream:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=dedab7f0d3137441a97fe7cf9b9ca5

(though we still have a useless cast in there; will fix as well).

May I ask what static checker you're using?

Thanks,

   Fred

Le 13/02/2018 à 09:12, Dan Carpenter a écrit :
Hello Frederic Barrat,

The patch aeddad1760ae: "ocxl: Add AFU interrupt support" from Jan
23, 2018, leads to the following static checker warning:

     drivers/misc/ocxl/file.c:163 afu_ioctl()
     warn: maybe return -EFAULT instead of the bytes remaining?

drivers/misc/ocxl/file.c
    111  static long afu_ioctl(struct file *file, unsigned int cmd,
    112                  unsigned long args)
    113  {
    114          struct ocxl_context *ctx = file->private_data;
    115          struct ocxl_ioctl_irq_fd irq_fd;
    116          u64 irq_offset;
    117          long rc;
    118
    119          pr_debug("%s for context %d, command %s\n", __func__, ctx->pasid,
    120                  CMD_STR(cmd));
    121
    122          if (ctx->status == CLOSED)
    123                  return -EIO;
    124
    125          switch (cmd) {
    126          case OCXL_IOCTL_ATTACH:
    127                  rc = afu_ioctl_attach(ctx,
    128                                  (struct ocxl_ioctl_attach __user *) args);
    129                  break;
    130
    131          case OCXL_IOCTL_IRQ_ALLOC:
    132                  rc = ocxl_afu_irq_alloc(ctx, &irq_offset);
    133                  if (!rc) {
    134                          rc = copy_to_user((u64 __user *) args, &irq_offset,
    135                                          sizeof(irq_offset));
    136                          if (rc)
                                     ^^
copy_to_user() returns the number of bytes remaining but we want to
return -EFAULT on error.

    137                                  ocxl_afu_irq_free(ctx, irq_offset);
    138                  }
    139                  break;
    140

     drivers/misc/ocxl/file.c:320 afu_read()
     warn: unsigned 'used' is never less than zero.

drivers/misc/ocxl/file.c
    279          ssize_t rc;
    280          size_t used = 0;
                 ^^^^^^
This should be ssize_t

    281          DEFINE_WAIT(event_wait);
    282
    283          memset(&header, 0, sizeof(header));
    284
    285          /* Require offset to be 0 */
    286          if (*off != 0)
    287                  return -EINVAL;
    288
    289          if (count < (sizeof(struct ocxl_kernel_event_header) +
    290                          AFU_EVENT_BODY_MAX_SIZE))
    291                  return -EINVAL;
    292
    293          for (;;) {
    294                  prepare_to_wait(&ctx->events_wq, &event_wait,
    295                                  TASK_INTERRUPTIBLE);
    296
    297                  if (afu_events_pending(ctx))
    298                          break;
    299
    300                  if (ctx->status == CLOSED)
    301                          break;
    302
    303                  if (file->f_flags & O_NONBLOCK) {
    304                          finish_wait(&ctx->events_wq, &event_wait);
    305                          return -EAGAIN;
    306                  }
    307
    308                  if (signal_pending(current)) {
    309                          finish_wait(&ctx->events_wq, &event_wait);
    310                          return -ERESTARTSYS;
    311                  }
    312
    313                  schedule();
    314          }
    315
    316          finish_wait(&ctx->events_wq, &event_wait);
    317
    318          if (has_xsl_error(ctx)) {
    319                  used = append_xsl_error(ctx, &header, buf + sizeof(header));
    320                  if (used < 0)
                             ^^^^^^^^
Impossible.

    321                          return used;
    322          }
    323
    324          if (!afu_events_pending(ctx))
    325                  header.flags |= OCXL_KERNEL_EVENT_FLAG_LAST;
    326
    327          if (copy_to_user(buf, &header, sizeof(header)))
    328                  return -EFAULT;
    329
    330          used += sizeof(header);
    331
    332          rc = (ssize_t) used;
                      ^^^^^^^^^^^^^^
You could remove the cast.

    333          return rc;
    334  }

regards,
dan carpenter

Re: [bug report] ocxl: Add AFU interrupt support

From: Dan Carpenter <hidden>
Date: 2018-02-14 07:29:55

On Tue, Feb 13, 2018 at 08:29:26PM +0100, Frederic Barrat wrote:
Hi,

Thanks for the report. I'll fix the first issue. The 2nd is already on its
way to upstream:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=dedab7f0d3137441a97fe7cf9b9ca5

(though we still have a useless cast in there; will fix as well).

May I ask what static checker you're using?
These are Smatch warnings.

regards,
dan carpenter
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help