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

[Frederic Barrat <fbarrat@linux.vnet.ibm.com>]

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
>