* [PATCH] scsi: avoid a double-fetch and a redundant copy
@ 2018-12-25 20:15 Kangjie Lu
2018-12-25 21:11 ` kbuild test robot
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Kangjie Lu @ 2018-12-25 20:15 UTC (permalink / raw)
To: kjlu
Cc: pakki001, Doug Gilbert, James E.J. Bottomley, Martin K. Petersen,
linux-scsi, linux-kernel
What we need is only "pack_id", so do not create a heap object or copy
the whole object in. The fix efficiently copies "pack_id" only.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/scsi/sg.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c6ad00703c5b..4dacbfffd113 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -446,16 +446,8 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
}
if (old_hdr->reply_len < 0) {
if (count >= SZ_SG_IO_HDR) {
- sg_io_hdr_t *new_hdr;
- new_hdr = kmalloc(SZ_SG_IO_HDR, GFP_KERNEL);
- if (!new_hdr) {
- retval = -ENOMEM;
- goto free_old_hdr;
- }
- retval =__copy_from_user
- (new_hdr, buf, SZ_SG_IO_HDR);
- req_pack_id = new_hdr->pack_id;
- kfree(new_hdr);
+ retval = get_user(req_pack_id,
+ &((sg_io_hdr_t *)buf->pack_id));
if (retval) {
retval = -EFAULT;
goto free_old_hdr;
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi: avoid a double-fetch and a redundant copy
2018-12-25 20:15 [PATCH] scsi: avoid a double-fetch and a redundant copy Kangjie Lu
@ 2018-12-25 21:11 ` kbuild test robot
2018-12-25 21:12 ` kbuild test robot
2018-12-26 6:49 ` Douglas Gilbert
2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2018-12-25 21:11 UTC (permalink / raw)
To: Kangjie Lu
Cc: kbuild-all, kjlu, pakki001, Doug Gilbert, James E.J. Bottomley,
Martin K. Petersen, linux-scsi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 10277 bytes --]
Hi Kangjie,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on scsi/for-next]
[also build test ERROR on v4.20 next-20181224]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kangjie-Lu/scsi-avoid-a-double-fetch-and-a-redundant-copy/20181226-042018
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-randconfig-x079-201851 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from include/linux/uaccess.h:14:0,
from include/linux/poll.h:12,
from drivers//scsi/sg.c:42:
drivers//scsi/sg.c: In function 'sg_read':
>> drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:138:41: note: in definition of macro '__inttype'
__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
^
>> drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
>> arch/x86/include/asm/uaccess.h:138:12: error: first argument to '__builtin_choose_expr' not a constant
__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
^
>> arch/x86/include/asm/uaccess.h:174:11: note: in expansion of macro '__inttype'
register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
^~~~~~~~~
>> drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
>> drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:180:15: note: in definition of macro 'get_user'
: "0" (ptr), "i" (sizeof(*(ptr)))); \
^~~
>> drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:180:35: note: in definition of macro 'get_user'
: "0" (ptr), "i" (sizeof(*(ptr)))); \
^~~
>> drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:181:30: note: in definition of macro 'get_user'
(x) = (__force __typeof__(*(ptr))) __val_gu; \
^~~
--
In file included from include/linux/uaccess.h:14:0,
from include/linux/poll.h:12,
from drivers/scsi/sg.c:42:
drivers/scsi/sg.c: In function 'sg_read':
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:138:41: note: in definition of macro '__inttype'
__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
^
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
>> arch/x86/include/asm/uaccess.h:138:12: error: first argument to '__builtin_choose_expr' not a constant
__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
^
>> arch/x86/include/asm/uaccess.h:174:11: note: in expansion of macro '__inttype'
register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
^~~~~~~~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:180:15: note: in definition of macro 'get_user'
: "0" (ptr), "i" (sizeof(*(ptr)))); \
^~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:180:35: note: in definition of macro 'get_user'
: "0" (ptr), "i" (sizeof(*(ptr)))); \
^~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/x86/include/asm/uaccess.h:181:30: note: in definition of macro 'get_user'
(x) = (__force __typeof__(*(ptr))) __val_gu; \
^~~
vim +/pack_id +450 drivers//scsi/sg.c
412
413 static ssize_t
414 sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
415 {
416 Sg_device *sdp;
417 Sg_fd *sfp;
418 Sg_request *srp;
419 int req_pack_id = -1;
420 sg_io_hdr_t *hp;
421 struct sg_header *old_hdr = NULL;
422 int retval = 0;
423
424 /*
425 * This could cause a response to be stranded. Close the associated
426 * file descriptor to free up any resources being held.
427 */
428 retval = sg_check_file_access(filp, __func__);
429 if (retval)
430 return retval;
431
432 if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
433 return -ENXIO;
434 SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp,
435 "sg_read: count=%d\n", (int) count));
436
437 if (!access_ok(VERIFY_WRITE, buf, count))
438 return -EFAULT;
439 if (sfp->force_packid && (count >= SZ_SG_HEADER)) {
440 old_hdr = kmalloc(SZ_SG_HEADER, GFP_KERNEL);
441 if (!old_hdr)
442 return -ENOMEM;
443 if (__copy_from_user(old_hdr, buf, SZ_SG_HEADER)) {
444 retval = -EFAULT;
445 goto free_old_hdr;
446 }
447 if (old_hdr->reply_len < 0) {
448 if (count >= SZ_SG_IO_HDR) {
> 449 retval = get_user(req_pack_id,
> 450 &((sg_io_hdr_t *)buf->pack_id));
451 if (retval) {
452 retval = -EFAULT;
453 goto free_old_hdr;
454 }
455 }
456 } else
457 req_pack_id = old_hdr->pack_id;
458 }
459 srp = sg_get_rq_mark(sfp, req_pack_id);
460 if (!srp) { /* now wait on packet to arrive */
461 if (atomic_read(&sdp->detaching)) {
462 retval = -ENODEV;
463 goto free_old_hdr;
464 }
465 if (filp->f_flags & O_NONBLOCK) {
466 retval = -EAGAIN;
467 goto free_old_hdr;
468 }
469 retval = wait_event_interruptible(sfp->read_wait,
470 (atomic_read(&sdp->detaching) ||
471 (srp = sg_get_rq_mark(sfp, req_pack_id))));
472 if (atomic_read(&sdp->detaching)) {
473 retval = -ENODEV;
474 goto free_old_hdr;
475 }
476 if (retval) {
477 /* -ERESTARTSYS as signal hit process */
478 goto free_old_hdr;
479 }
480 }
481 if (srp->header.interface_id != '\0') {
482 retval = sg_new_read(sfp, buf, count, srp);
483 goto free_old_hdr;
484 }
485
486 hp = &srp->header;
487 if (old_hdr == NULL) {
488 old_hdr = kmalloc(SZ_SG_HEADER, GFP_KERNEL);
489 if (! old_hdr) {
490 retval = -ENOMEM;
491 goto free_old_hdr;
492 }
493 }
494 memset(old_hdr, 0, SZ_SG_HEADER);
495 old_hdr->reply_len = (int) hp->timeout;
496 old_hdr->pack_len = old_hdr->reply_len; /* old, strange behaviour */
497 old_hdr->pack_id = hp->pack_id;
498 old_hdr->twelve_byte =
499 ((srp->data.cmd_opcode >= 0xc0) && (12 == hp->cmd_len)) ? 1 : 0;
500 old_hdr->target_status = hp->masked_status;
501 old_hdr->host_status = hp->host_status;
502 old_hdr->driver_status = hp->driver_status;
503 if ((CHECK_CONDITION & hp->masked_status) ||
504 (DRIVER_SENSE & hp->driver_status))
505 memcpy(old_hdr->sense_buffer, srp->sense_b,
506 sizeof (old_hdr->sense_buffer));
507 switch (hp->host_status) {
508 /* This setup of 'result' is for backward compatibility and is best
509 ignored by the user who should use target, host + driver status */
510 case DID_OK:
511 case DID_PASSTHROUGH:
512 case DID_SOFT_ERROR:
513 old_hdr->result = 0;
514 break;
515 case DID_NO_CONNECT:
516 case DID_BUS_BUSY:
517 case DID_TIME_OUT:
518 old_hdr->result = EBUSY;
519 break;
520 case DID_BAD_TARGET:
521 case DID_ABORT:
522 case DID_PARITY:
523 case DID_RESET:
524 case DID_BAD_INTR:
525 old_hdr->result = EIO;
526 break;
527 case DID_ERROR:
528 old_hdr->result = (srp->sense_b[0] == 0 &&
529 hp->masked_status == GOOD) ? 0 : EIO;
530 break;
531 default:
532 old_hdr->result = EIO;
533 break;
534 }
535
536 /* Now copy the result back to the user buffer. */
537 if (count >= SZ_SG_HEADER) {
538 if (__copy_to_user(buf, old_hdr, SZ_SG_HEADER)) {
539 retval = -EFAULT;
540 goto free_old_hdr;
541 }
542 buf += SZ_SG_HEADER;
543 if (count > old_hdr->reply_len)
544 count = old_hdr->reply_len;
545 if (count > SZ_SG_HEADER) {
546 if (sg_read_oxfer(srp, buf, count - SZ_SG_HEADER)) {
547 retval = -EFAULT;
548 goto free_old_hdr;
549 }
550 }
551 } else
552 count = (old_hdr->result == 0) ? 0 : -EIO;
553 sg_finish_rem_req(srp);
554 sg_remove_request(sfp, srp);
555 retval = count;
556 free_old_hdr:
557 kfree(old_hdr);
558 return retval;
559 }
560
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30978 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi: avoid a double-fetch and a redundant copy
2018-12-25 20:15 [PATCH] scsi: avoid a double-fetch and a redundant copy Kangjie Lu
2018-12-25 21:11 ` kbuild test robot
@ 2018-12-25 21:12 ` kbuild test robot
2018-12-26 6:49 ` Douglas Gilbert
2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2018-12-25 21:12 UTC (permalink / raw)
To: Kangjie Lu
Cc: kbuild-all, kjlu, pakki001, Doug Gilbert, James E.J. Bottomley,
Martin K. Petersen, linux-scsi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 20506 bytes --]
Hi Kangjie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.20 next-20181224]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kangjie-Lu/scsi-avoid-a-double-fetch-and-a-redundant-copy/20181226-042018
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=m68k
All warnings (new ones prefixed by >>):
In file included from arch/m68k/include/asm/uaccess.h:5:0,
from include/linux/uaccess.h:14,
from include/linux/poll.h:12,
from drivers/scsi/sg.c:42:
drivers/scsi/sg.c: In function 'sg_read':
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:134:19: note: in definition of macro '__get_user'
switch (sizeof(*(ptr))) { \
^~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:126:12: note: in definition of macro '__get_user_asm'
: "m" (*(ptr)), "i" (err)); \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:127:26: note: in definition of macro '__get_user_asm'
(x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val; \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:126:12: note: in definition of macro '__get_user_asm'
: "m" (*(ptr)), "i" (err)); \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:127:26: note: in definition of macro '__get_user_asm'
(x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val; \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:126:12: note: in definition of macro '__get_user_asm'
: "m" (*(ptr)), "i" (err)); \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:127:26: note: in definition of macro '__get_user_asm'
(x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val; \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:145:27: note: in definition of macro '__get_user'
const void *__gu_ptr = (ptr); \
^~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers/scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:148:17: note: in definition of macro '__get_user'
__typeof__(*(ptr)) t; \
^~~
drivers/scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
--
In file included from arch/m68k/include/asm/uaccess.h:5:0,
from include/linux/uaccess.h:14,
from include/linux/poll.h:12,
from drivers//scsi/sg.c:42:
drivers//scsi/sg.c: In function 'sg_read':
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:134:19: note: in definition of macro '__get_user'
switch (sizeof(*(ptr))) { \
^~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:126:12: note: in definition of macro '__get_user_asm'
: "m" (*(ptr)), "i" (err)); \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:127:26: note: in definition of macro '__get_user_asm'
(x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val; \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:126:12: note: in definition of macro '__get_user_asm'
: "m" (*(ptr)), "i" (err)); \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:127:26: note: in definition of macro '__get_user_asm'
(x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val; \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:126:12: note: in definition of macro '__get_user_asm'
: "m" (*(ptr)), "i" (err)); \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:127:26: note: in definition of macro '__get_user_asm'
(x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val; \
^~~
>> arch/m68k/include/asm/uaccess_mm.h:180:26: note: in expansion of macro '__get_user'
#define get_user(x, ptr) __get_user(x, ptr)
^~~~~~~~~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:145:27: note: in definition of macro '__get_user'
const void *__gu_ptr = (ptr); \
^~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
drivers//scsi/sg.c:450:27: error: request for member 'pack_id' in something not a structure or union
&((sg_io_hdr_t *)buf->pack_id));
^
arch/m68k/include/asm/uaccess_mm.h:148:17: note: in definition of macro '__get_user'
__typeof__(*(ptr)) t; \
^~~
drivers//scsi/sg.c:449:14: note: in expansion of macro 'get_user'
retval = get_user(req_pack_id,
^~~~~~~~
vim +/__get_user +180 arch/m68k/include/asm/uaccess_mm.h
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 107
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 108
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 109 #define __get_user_asm(res, x, ptr, type, bwl, reg, err) ({ \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 110 type __gu_val; \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 111 asm volatile ("\n" \
e08d703cc arch/m68k/include/asm/uaccess_mm.h Greg Ungerer 2011-10-14 112 "1: "MOVES"."#bwl" %2,%1\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 113 "2:\n" \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 114 " .section .fixup,\"ax\"\n" \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 115 " .even\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 116 "10: move.l %3,%0\n" \
e08d703cc arch/m68k/include/asm/uaccess_mm.h Greg Ungerer 2011-10-14 117 " sub.l %1,%1\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 118 " jra 2b\n" \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 119 " .previous\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 120 "\n" \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 121 " .section __ex_table,\"a\"\n" \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 122 " .align 4\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 123 " .long 1b,10b\n" \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 124 " .previous" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 125 : "+d" (res), "=&" #reg (__gu_val) \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 @126 : "m" (*(ptr)), "i" (err)); \
09a2f7cf6 arch/m68k/include/asm/uaccess_mm.h Michael S. Tsirkin 2014-12-12 127 (x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val; \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 128 })
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 129
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 130 #define __get_user(x, ptr) \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 131 ({ \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 132 int __gu_err = 0; \
11c40f8a6 include/asm-m68k/uaccess.h Al Viro 2006-01-12 133 __chk_user_ptr(ptr); \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 134 switch (sizeof(*(ptr))) { \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 135 case 1: \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 136 __get_user_asm(__gu_err, x, ptr, u8, b, d, -EFAULT); \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 137 break; \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 138 case 2: \
631d8b674 arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2013-06-09 139 __get_user_asm(__gu_err, x, ptr, u16, w, r, -EFAULT); \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 140 break; \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 141 case 4: \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 142 __get_user_asm(__gu_err, x, ptr, u32, l, r, -EFAULT); \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 143 break; \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 144 case 8: { \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 145 const void *__gu_ptr = (ptr); \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 146 union { \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 147 u64 l; \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 148 __typeof__(*(ptr)) t; \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 149 } __gu_val; \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 150 asm volatile ("\n" \
e08d703cc arch/m68k/include/asm/uaccess_mm.h Greg Ungerer 2011-10-14 151 "1: "MOVES".l (%2)+,%1\n" \
e08d703cc arch/m68k/include/asm/uaccess_mm.h Greg Ungerer 2011-10-14 152 "2: "MOVES".l (%2),%R1\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 153 "3:\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 154 " .section .fixup,\"ax\"\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 155 " .even\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 156 "10: move.l %3,%0\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 157 " sub.l %1,%1\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 158 " sub.l %R1,%R1\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 159 " jra 3b\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 160 " .previous\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 161 "\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 162 " .section __ex_table,\"a\"\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 163 " .align 4\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 164 " .long 1b,10b\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 165 " .long 2b,10b\n" \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 166 " .previous" \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 167 : "+d" (__gu_err), "=&r" (__gu_val.l), \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 168 "+a" (__gu_ptr) \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 169 : "i" (-EFAULT) \
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 170 : "memory"); \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 171 (x) = __gu_val.t; \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 172 break; \
7124330da arch/m68k/include/asm/uaccess_mm.h Geert Uytterhoeven 2018-05-14 173 } \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 174 default: \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 175 __gu_err = __get_user_bad(); \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 176 break; \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 177 } \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 178 __gu_err; \
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 179 })
d94af931a include/asm-m68k/uaccess.h Roman Zippel 2006-06-23 @180 #define get_user(x, ptr) __get_user(x, ptr)
^1da177e4 include/asm-m68k/uaccess.h Linus Torvalds 2005-04-16 181
:::::: The code at line 180 was first introduced by commit
:::::: d94af931af42152e34539dd4782b1724084a89fb [PATCH] m68k: clean up uaccess.h
:::::: TO: Roman Zippel <zippel@linux-m68k.org>
:::::: CC: Linus Torvalds <torvalds@g5.osdl.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47651 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi: avoid a double-fetch and a redundant copy
2018-12-25 20:15 [PATCH] scsi: avoid a double-fetch and a redundant copy Kangjie Lu
2018-12-25 21:11 ` kbuild test robot
2018-12-25 21:12 ` kbuild test robot
@ 2018-12-26 6:49 ` Douglas Gilbert
2019-01-09 7:25 ` [PATCH v2] " Kangjie Lu
2 siblings, 1 reply; 5+ messages in thread
From: Douglas Gilbert @ 2018-12-26 6:49 UTC (permalink / raw)
To: Kangjie Lu
Cc: pakki001, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel
On 2018-12-25 3:15 p.m., Kangjie Lu wrote:
> What we need is only "pack_id", so do not create a heap object or copy
> the whole object in. The fix efficiently copies "pack_id" only.
Now this looks like a worthwhile optimization, in some pretty tricky
code. I can't see a security angle in it. Did you test it?
Well the code as presented doesn't compile and the management takes a
dim view of that.
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
> drivers/scsi/sg.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index c6ad00703c5b..4dacbfffd113 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -446,16 +446,8 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
> }
> if (old_hdr->reply_len < 0) {
> if (count >= SZ_SG_IO_HDR) {
> - sg_io_hdr_t *new_hdr;
> - new_hdr = kmalloc(SZ_SG_IO_HDR, GFP_KERNEL);
> - if (!new_hdr) {
> - retval = -ENOMEM;
> - goto free_old_hdr;
> - }
> - retval =__copy_from_user
> - (new_hdr, buf, SZ_SG_IO_HDR);
> - req_pack_id = new_hdr->pack_id;
> - kfree(new_hdr);
> + retval = get_user(req_pack_id,
> + &((sg_io_hdr_t *)buf->pack_id));
The '->' binds higher then the cast and since buf is a 'char *' it doesn't
have a member called pack_id .
Hopefully your drive to remove redundancy went a little too far and removed
the required (but missing) parentheses binding the cast to 'buf'.
> if (retval) {
> retval = -EFAULT;
> goto free_old_hdr;
>
Good work, silly mistake, but its got me thinking, the heap allocation can be
replaced by stack since its short. The code in this area is more tricky in
the v4 driver because I want to specifically exclude the sg_io_v4 (aka v4)
interface being sent through write(2)/read(2). The way to do that is to read
the first 32 bit integer which should be 'S' or v3, 'Q' for v4.
Hmm, just looking further along my mailer I see the kbuild test robot
has picked up the error and you have presented another patch which also
won't compile. Please stop doing that; apply your patch to kernel source
and compile it _before_ sending it to this list.
Doug Gilbert
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] scsi: avoid a double-fetch and a redundant copy
2018-12-26 6:49 ` Douglas Gilbert
@ 2019-01-09 7:25 ` Kangjie Lu
0 siblings, 0 replies; 5+ messages in thread
From: Kangjie Lu @ 2019-01-09 7:25 UTC (permalink / raw)
To: kjlu
Cc: pakki001, Doug Gilbert, James E.J. Bottomley, Martin K. Petersen,
linux-scsi, linux-kernel
What we need is only "pack_id", so do not create a heap object or copy
the whole object in. The fix efficiently copies "pack_id" only and
also avoids double-fetch.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/scsi/sg.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c6ad00703c5b..13662c41058a 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -446,16 +446,8 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
}
if (old_hdr->reply_len < 0) {
if (count >= SZ_SG_IO_HDR) {
- sg_io_hdr_t *new_hdr;
- new_hdr = kmalloc(SZ_SG_IO_HDR, GFP_KERNEL);
- if (!new_hdr) {
- retval = -ENOMEM;
- goto free_old_hdr;
- }
- retval =__copy_from_user
- (new_hdr, buf, SZ_SG_IO_HDR);
- req_pack_id = new_hdr->pack_id;
- kfree(new_hdr);
+ retval = get_user(req_pack_id,
+ &((sg_io_hdr_t *)buf)->pack_id);
if (retval) {
retval = -EFAULT;
goto free_old_hdr;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-09 7:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-25 20:15 [PATCH] scsi: avoid a double-fetch and a redundant copy Kangjie Lu
2018-12-25 21:11 ` kbuild test robot
2018-12-25 21:12 ` kbuild test robot
2018-12-26 6:49 ` Douglas Gilbert
2019-01-09 7:25 ` [PATCH v2] " Kangjie Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).