staging/lustre: clean up and remove libcfs/linux/linux-fs.c

Quoted 30 lines · Peng Tao

int libcfs_kkuc_msg_put(struct file *filp, void *payload)

 {
      struct kuc_hdr *kuch = (struct kuc_hdr *)payload;

(quoted diff) +14 -4

+     ssize_t count = kuch->kuc_msglen;
+     loff_t offset = 0;
+     mm_segment_t fs;
      int rc = -ENOSYS;
 
      if (filp == NULL || IS_ERR(filp))

@@ -165,11 +168,18 @@ int libcfs_kkuc_msg_put(struct file *filp, void *payload)
              return -ENOSYS;
      }
 
-     {
-             loff_t offset = 0;
-             rc = filp_user_write(filp, payload, kuch->kuc_msglen,
-                                  &offset);
+     fs = get_fs();
+     set_fs(KERNEL_DS);
+     while ((ssize_t)count > 0) {
+             rc = vfs_write(filp, (const void __user *)payload,
+                            count, &offset);
+             if (rc < 0)
+                     break;
+             count -= rc;
+             payload += rc;
+             rc = 0;
      }
+     set_fs(fs);
 

Review int libcfs_kkuc_msg_put(struct file *filp, void *payload) · line 185

Quoted 2 lines

(quoted diff) +0 -0

      if (rc < 0)
              CWARN("message send failed (%d)\n", rc);

This was all there in the original code, it has just been shifted. Still, I had some questions about it. "payload" is a pointer to kernel stack memory, wouldn't the access_ok() check in vfs_write() fail every time on x86?

Some of the casting is not needed. No need to cast "count" because it is already ssize_t. I haven't tested but I think Sparse will complain about the __user cast until __force is added. No need for the cast to const.

I worry about partial writes and that this could be a forever loop but I don't know enough about anything to say if that's possible. Probably not.

Quoted 31 lines · Dan Carpenter

Quoted 30 lines · Peng Tao

int libcfs_kkuc_msg_put(struct file *filp, void *payload)

 {
      struct kuc_hdr *kuch = (struct kuc_hdr *)payload;

(quoted diff) +14 -4

+     ssize_t count = kuch->kuc_msglen;
+     loff_t offset = 0;
+     mm_segment_t fs;
      int rc = -ENOSYS;
 
      if (filp == NULL || IS_ERR(filp))

@@ -165,11 +168,18 @@ int libcfs_kkuc_msg_put(struct file *filp, void *payload)
              return -ENOSYS;
      }
 
-     {
-             loff_t offset = 0;
-             rc = filp_user_write(filp, payload, kuch->kuc_msglen,
-                                  &offset);
+     fs = get_fs();
+     set_fs(KERNEL_DS);
+     while ((ssize_t)count > 0) {
+             rc = vfs_write(filp, (const void __user *)payload,
+                            count, &offset);
+             if (rc < 0)
+                     break;
+             count -= rc;
+             payload += rc;
+             rc = 0;
      }
+     set_fs(fs);
 

Quoted 8 lines

Review int libcfs_kkuc_msg_put(struct file *filp, void *payload) · line 185

Quoted 2 lines

(quoted diff) +0 -0

      if (rc < 0)
              CWARN("message send failed (%d)\n", rc);

This was all there in the original code, it has just been shifted. Still, I had some questions about it. "payload" is a pointer to kernel stack memory, wouldn't the access_ok() check in vfs_write() fail every time on x86?

Thanks for reviewing. I am not familiar with access_ok() but I think you are right and I'll test to see if it really breaks. In the meantime, I took a look at other kernel callers of vfs_write() and btrfs is also calling vfs_write() to write kernel stack memory (fs/btrfs/send.c: send_header->write_buf->vfs_write). So I CC'ed btrfs list in case someone knows better than I do.

Quoted 5 lines

Some of the casting is not needed. No need to cast "count" because it is already ssize_t. I haven't tested but I think Sparse will complain about the __user cast until __force is added. No need for the cast to const.

Thanks. I will fix it.

Quoted 4 lines

I worry about partial writes and that this could be a forever loop but I don't know enough about anything to say if that's possible. Probably not.

For partial writes, both payload and count are advanced properly. So it won't loop forever.

Thanks,
Tao