Re: [LTP] LTP: syscalls: regression on mainline - ioctl_loop01 mknod07 setns01
Hi Martign
Also for your kernel commit,
lo->lo_flags |= prev_lo_flags & ~LOOP_SET_STATUS_SETTABLE_FLAGS;
lo->lo_flags |= prev_lo_flags & ~LOOP_SET_STATUS_CLEARABLE_FLAGS;
since ~LOOP_SET_STATUS_SETTABLE_FLAGS has been included in
~LOOP_SET_STATUS_CLEARABLE_FLAGS, do we still need the previous step?
What do you think about it?
Best Regards
Yang Xu
Hey Yang,
On Fri, Jun 5, 2020 at 10:59 AM Yang Xu wrote:
Hi Martijn
Sorry for noise. I see your patch in here[1] . I will modify
ioctl_loop01 to test that LO_FLAGS_PARTSCAN can not clear and
LO_FLAGS_AUTOCLEAR can be clear.
Thanks, that would indeed be useful.
ps: Giving the url of patch is better so that other people doesn't need
to investigate it again.
[1]https://patchwork.kernel.org/patch/11588321/
Ok, will do next time!
Best,
Martijn
Best Regards
Yang Xu
Hi Martijn
Hi Naresh,
I just sent a patch and cc'd you. I verified all the loop tests pass
again with that patch.
I think you want to say "without". I verified the ioctl_loop01 fails
with faf1d25440 ("loop: Clean up LOOP_SET_STATUS lo_flags handling").
This kernel commit breaks old behaviour(if old flag all 0, new flag is
always 0 regradless your flag setting).
I think we should modify code as below:
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 13518ba191f5..c6ba8cf486ce 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1364,11 +1364,9 @@ loop_set_status(struct loop_device *lo, const
struct loop_info64 *info)
if (err)
goto out_unfreeze;
- /* Mask out flags that can't be set using LOOP_SET_STATUS. */
- lo->lo_flags &= ~LOOP_SET_STATUS_SETTABLE_FLAGS;
- /* For those flags, use the previous values instead */
- lo->lo_flags |= prev_lo_flags & ~LOOP_SET_STATUS_SETTABLE_FLAGS;
- /* For flags that can't be cleared, use previous values too */
+ /* Mask out flags that can be set using LOOP_SET_STATUS. */
+ lo->lo_flags &= LOOP_SET_STATUS_SETTABLE_FLAGS;
+ /* For flags that can't be cleared, use previous values. */
lo->lo_flags |= prev_lo_flags &~LOOP_SET_STATUS_CLEARABLE_FLAGS;
Best Regards
Yang Xu
Thanks,
Martijn
On Thu, Jun 4, 2020 at 9:10 PM Martijn Coenen wrote:
Hi Naresh,
I suspect the loop failures are due to
faf1d25440d6ad06d509dada4b6fe62fea844370 ("loop: Clean up
LOOP_SET_STATUS lo_flags handling"), I will investigate and get back
to you.
Thanks,
Martijn
On Thu, Jun 4, 2020 at 7:19 PM Naresh Kamboju
wrote:
+ linux-bl...@vger.kernel.org
On Thu, 4 Jun 2020 at 22:47, Naresh Kamboju
wrote:
Following three test cases reported as regression on Linux mainline
kernel
on x86_64, arm64, arm and i386
ltp-syscalls-tests:
* ioctl_loop01
* mknod07
* setns01
git repo:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git branch: master
GOOD:
git commit: b23c4771ff62de8ca9b5e4a2d64491b2fb6f8f69
git describe: v5.7-1230-gb23c4771ff62
BAD:
git commit: 1ee08de1e234d95b5b4f866878b72fceb5372904
git describe: v5.7-3523-g1ee08de1e234
kernel-config:
https://builds.tuxbuild.com/U3bU0dMA62OVHb4DvZIVuw/kernel.config
We are investigating these failures.
tst_test.c:906: CONF: btrfs driver not available
tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
tst_device.c:88: INFO: Found free device 1 '/dev/loop1'
ioctl_loop01.c:49: PASS: /sys/block/loop1/loop/partscan = 0
[ 1073.639677] loop_set_status: loop1 () has still dirty pages
(nrpages=1)
ioctl_loop01.c:50: PASS: /sys/block/loop1/loop/autoclear = 0
ioctl_loop01.c:51: PASS: /sys/block/loop1/loop/backing_file =
'/scratch/ltp-mnIdulzriQ/9cPtLQ/test.img'
ioctl_loop01.c:63: FAIL: expect 12 but got 17
ioctl_loop01.c:67: FAIL: /sys/block/loop1/loop/partscan != 1 got 0
ioctl_loop01.c:68: FAIL: /sys/block/loop1/loop/autoclear != 1 got 0
ioctl_loop01.c:79: FAIL: access /dev/loop1p1 fails
[ 1073.679678] loop_set_status: loop1 () has still dirty pages
(nrpages=1)
ioctl_loop01.c:85: FAIL: access /sys/block/loop1/loop1p1 fails
HINT: You _MAY_ be missing kernel fixes, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=10c70d95c0f2
mke2fs 1.43.8 (1-Jan-2018)
[ 1264.711379] EXT4-fs (loop0): mounting ext2 file system using the
ext4 subsystem
[ 1264.716642] EXT4-fs (loop0): mounted filesystem without journal.
Opts: (null)
mknod07 0 TINFO : Using test device LTP_DEV='/dev/loop0'
mknod07 0 TINFO : Formatting /dev/loop0 with ext2 opts=''
extra opts=''
mknod07 1 TPASS : mknod failed as expected:
TEST_ERRNO=EACCES(13): Permission denied
mknod07 2 TPASS : mknod failed as expected:
TEST_ERRNO=EACCES(13): Permission denied
mknod07 3 TFAIL : mknod07.c:155: mknod succeeded unexpectedly
mknod07 4 TPASS : mknod failed as expected:
TEST_ERRNO=EPERM(1): Operation no
Re: [LTP] LTP: syscalls: regression on mainline - ioctl_loop01 mknod07 setns01
Hi Martijn
Sorry for noise. I see your patch in here[1] . I will modify
ioctl_loop01 to test that LO_FLAGS_PARTSCAN can not clear and
LO_FLAGS_AUTOCLEAR can be clear.
ps: Giving the url of patch is better so that other people doesn't need
to investigate it again.
[1]https://patchwork.kernel.org/patch/11588321/
Best Regards
Yang Xu
Hi Martijn
Hi Naresh,
I just sent a patch and cc'd you. I verified all the loop tests pass
again with that patch.
I think you want to say "without". I verified the ioctl_loop01 fails
with faf1d25440 ("loop: Clean up LOOP_SET_STATUS lo_flags handling").
This kernel commit breaks old behaviour(if old flag all 0, new flag is
always 0 regradless your flag setting).
I think we should modify code as below:
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 13518ba191f5..c6ba8cf486ce 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1364,11 +1364,9 @@ loop_set_status(struct loop_device *lo, const
struct loop_info64 *info)
if (err)
goto out_unfreeze;
- /* Mask out flags that can't be set using LOOP_SET_STATUS. */
- lo->lo_flags &= ~LOOP_SET_STATUS_SETTABLE_FLAGS;
- /* For those flags, use the previous values instead */
- lo->lo_flags |= prev_lo_flags & ~LOOP_SET_STATUS_SETTABLE_FLAGS;
- /* For flags that can't be cleared, use previous values too */
+ /* Mask out flags that can be set using LOOP_SET_STATUS. */
+ lo->lo_flags &= LOOP_SET_STATUS_SETTABLE_FLAGS;
+ /* For flags that can't be cleared, use previous values. */
lo->lo_flags |= prev_lo_flags &~LOOP_SET_STATUS_CLEARABLE_FLAGS;
Best Regards
Yang Xu
Thanks,
Martijn
On Thu, Jun 4, 2020 at 9:10 PM Martijn Coenen wrote:
Hi Naresh,
I suspect the loop failures are due to
faf1d25440d6ad06d509dada4b6fe62fea844370 ("loop: Clean up
LOOP_SET_STATUS lo_flags handling"), I will investigate and get back
to you.
Thanks,
Martijn
On Thu, Jun 4, 2020 at 7:19 PM Naresh Kamboju
wrote:
+ linux-bl...@vger.kernel.org
On Thu, 4 Jun 2020 at 22:47, Naresh Kamboju
wrote:
Following three test cases reported as regression on Linux mainline
kernel
on x86_64, arm64, arm and i386
ltp-syscalls-tests:
* ioctl_loop01
* mknod07
* setns01
git repo:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git branch: master
GOOD:
git commit: b23c4771ff62de8ca9b5e4a2d64491b2fb6f8f69
git describe: v5.7-1230-gb23c4771ff62
BAD:
git commit: 1ee08de1e234d95b5b4f866878b72fceb5372904
git describe: v5.7-3523-g1ee08de1e234
kernel-config:
https://builds.tuxbuild.com/U3bU0dMA62OVHb4DvZIVuw/kernel.config
We are investigating these failures.
tst_test.c:906: CONF: btrfs driver not available
tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
tst_device.c:88: INFO: Found free device 1 '/dev/loop1'
ioctl_loop01.c:49: PASS: /sys/block/loop1/loop/partscan = 0
[ 1073.639677] loop_set_status: loop1 () has still dirty pages
(nrpages=1)
ioctl_loop01.c:50: PASS: /sys/block/loop1/loop/autoclear = 0
ioctl_loop01.c:51: PASS: /sys/block/loop1/loop/backing_file =
'/scratch/ltp-mnIdulzriQ/9cPtLQ/test.img'
ioctl_loop01.c:63: FAIL: expect 12 but got 17
ioctl_loop01.c:67: FAIL: /sys/block/loop1/loop/partscan != 1 got 0
ioctl_loop01.c:68: FAIL: /sys/block/loop1/loop/autoclear != 1 got 0
ioctl_loop01.c:79: FAIL: access /dev/loop1p1 fails
[ 1073.679678] loop_set_status: loop1 () has still dirty pages
(nrpages=1)
ioctl_loop01.c:85: FAIL: access /sys/block/loop1/loop1p1 fails
HINT: You _MAY_ be missing kernel fixes, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=10c70d95c0f2
mke2fs 1.43.8 (1-Jan-2018)
[ 1264.711379] EXT4-fs (loop0): mounting ext2 file system using the
ext4 subsystem
[ 1264.716642] EXT4-fs (loop0): mounted filesystem without journal.
Opts: (null)
mknod07 0 TINFO : Using test device LTP_DEV='/dev/loop0'
mknod07 0 TINFO : Formatting /dev/loop0 with ext2 opts=''
extra opts=''
mknod07 1 TPASS : mknod failed as expected:
TEST_ERRNO=EACCES(13): Permission denied
mknod07 2 TPASS : mknod failed as expected:
TEST_ERRNO=EACCES(13): Permission denied
mknod07 3 TFAIL : mknod07.c:155: mknod succeeded unexpectedly
mknod07 4 TPASS : mknod failed as expected:
TEST_ERRNO=EPERM(1): Operation not permitted
mknod07 5 TPASS : mknod failed as expected:
TEST_ERRNO=EROFS(30): Read-only file system
mknod07 6 TPASS : mknod failed as expected:
TEST_ERRNO=ELOOP(40): Too many levels of symbolic links
setns01 0 TINFO : ns_name=ipc, ns_fds[0]=6,
ns_types[0]=0x800
setns01 0 TINFO : ns_name=mnt, ns_fds[1]=7, ns_types[1]=0x2
setns01 0 TINFO : ns_name=net, ns_fds[2]=8,
ns_types[2]=0x4000
setns01 0 TINFO : ns_name=pid, ns_fds[3]=9,
ns_types[3
Re: LTP: syscalls: regression on mainline - ioctl_loop01 mknod07 setns01
Hi Martijn
Hi Naresh,
I just sent a patch and cc'd you. I verified all the loop tests pass
again with that patch.
I think you want to say "without". I verified the ioctl_loop01 fails
with faf1d25440 ("loop: Clean up LOOP_SET_STATUS lo_flags handling").
This kernel commit breaks old behaviour(if old flag all 0, new flag is
always 0 regradless your flag setting).
I think we should modify code as below:
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 13518ba191f5..c6ba8cf486ce 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1364,11 +1364,9 @@ loop_set_status(struct loop_device *lo, const
struct loop_info64 *info)
if (err)
goto out_unfreeze;
- /* Mask out flags that can't be set using LOOP_SET_STATUS. */
- lo->lo_flags &= ~LOOP_SET_STATUS_SETTABLE_FLAGS;
- /* For those flags, use the previous values instead */
- lo->lo_flags |= prev_lo_flags & ~LOOP_SET_STATUS_SETTABLE_FLAGS;
- /* For flags that can't be cleared, use previous values too */
+ /* Mask out flags that can be set using LOOP_SET_STATUS. */
+ lo->lo_flags &= LOOP_SET_STATUS_SETTABLE_FLAGS;
+ /* For flags that can't be cleared, use previous values. */
lo->lo_flags |= prev_lo_flags &~LOOP_SET_STATUS_CLEARABLE_FLAGS;
Best Regards
Yang Xu
Thanks,
Martijn
On Thu, Jun 4, 2020 at 9:10 PM Martijn Coenen wrote:
Hi Naresh,
I suspect the loop failures are due to
faf1d25440d6ad06d509dada4b6fe62fea844370 ("loop: Clean up
LOOP_SET_STATUS lo_flags handling"), I will investigate and get back
to you.
Thanks,
Martijn
On Thu, Jun 4, 2020 at 7:19 PM Naresh Kamboju wrote:
+ linux-bl...@vger.kernel.org
On Thu, 4 Jun 2020 at 22:47, Naresh Kamboju wrote:
Following three test cases reported as regression on Linux mainline kernel
on x86_64, arm64, arm and i386
ltp-syscalls-tests:
* ioctl_loop01
* mknod07
* setns01
git repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git branch: master
GOOD:
git commit: b23c4771ff62de8ca9b5e4a2d64491b2fb6f8f69
git describe: v5.7-1230-gb23c4771ff62
BAD:
git commit: 1ee08de1e234d95b5b4f866878b72fceb5372904
git describe: v5.7-3523-g1ee08de1e234
kernel-config: https://builds.tuxbuild.com/U3bU0dMA62OVHb4DvZIVuw/kernel.config
We are investigating these failures.
tst_test.c:906: CONF: btrfs driver not available
tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
tst_device.c:88: INFO: Found free device 1 '/dev/loop1'
ioctl_loop01.c:49: PASS: /sys/block/loop1/loop/partscan = 0
[ 1073.639677] loop_set_status: loop1 () has still dirty pages (nrpages=1)
ioctl_loop01.c:50: PASS: /sys/block/loop1/loop/autoclear = 0
ioctl_loop01.c:51: PASS: /sys/block/loop1/loop/backing_file =
'/scratch/ltp-mnIdulzriQ/9cPtLQ/test.img'
ioctl_loop01.c:63: FAIL: expect 12 but got 17
ioctl_loop01.c:67: FAIL: /sys/block/loop1/loop/partscan != 1 got 0
ioctl_loop01.c:68: FAIL: /sys/block/loop1/loop/autoclear != 1 got 0
ioctl_loop01.c:79: FAIL: access /dev/loop1p1 fails
[ 1073.679678] loop_set_status: loop1 () has still dirty pages (nrpages=1)
ioctl_loop01.c:85: FAIL: access /sys/block/loop1/loop1p1 fails
HINT: You _MAY_ be missing kernel fixes, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=10c70d95c0f2
mke2fs 1.43.8 (1-Jan-2018)
[ 1264.711379] EXT4-fs (loop0): mounting ext2 file system using the
ext4 subsystem
[ 1264.716642] EXT4-fs (loop0): mounted filesystem without journal. Opts: (null)
mknod07 0 TINFO : Using test device LTP_DEV='/dev/loop0'
mknod07 0 TINFO : Formatting /dev/loop0 with ext2 opts='' extra opts=''
mknod07 1 TPASS : mknod failed as expected:
TEST_ERRNO=EACCES(13): Permission denied
mknod07 2 TPASS : mknod failed as expected:
TEST_ERRNO=EACCES(13): Permission denied
mknod07 3 TFAIL : mknod07.c:155: mknod succeeded unexpectedly
mknod07 4 TPASS : mknod failed as expected:
TEST_ERRNO=EPERM(1): Operation not permitted
mknod07 5 TPASS : mknod failed as expected:
TEST_ERRNO=EROFS(30): Read-only file system
mknod07 6 TPASS : mknod failed as expected:
TEST_ERRNO=ELOOP(40): Too many levels of symbolic links
setns01 0 TINFO : ns_name=ipc, ns_fds[0]=6, ns_types[0]=0x800
setns01 0 TINFO : ns_name=mnt, ns_fds[1]=7, ns_types[1]=0x2
setns01 0 TINFO : ns_name=net, ns_fds[2]=8, ns_types[2]=0x4000
setns01 0 TINFO : ns_name=pid, ns_fds[3]=9, ns_types[3]=0x2000
setns01 0 TINFO : ns_name=uts, ns_fds[4]=10, ns_types[4]=0x400
setns01 0 TINFO : setns(-1, 0x800)
setns01 1 TPASS : invalid fd exp_errno=9
setns01 0 TINFO : setns(-1, 0x2)
setns01 2 TPASS : invalid fd exp_errno=9
setns01 0 TINFO : setns(-1, 0x4000)
setns01 3 TPASS : invalid fd exp_errno=9
Re: [LTP] [PATCH v4 3/3] syscalls/pipe2_03: Add new test for pipe2 O_DIRECT flag
Hi Linus On Sun, Apr 26, 2020 at 4:59 AM Li Wang wrote: From kernel code seems you are right. The pipe indeed takes use of PAGE_SIZE(ppc64le: 64kB) to split the writes data in the packetized mode (marked by O_DIRECT). But in the manual page, O_DIRECT indicates us the PIPE_BUF is the correct atomic unit. The manual is correct. PIPE_BUF is the size we _guarantee_ can be used atomically. The fact that in practice we do have bigger buffers on some platforms is an implementation detail. Yes, that implementation detail can be visible, but basically any test code that tries to test for "what if we use a bigger bug that PIPE_BUF" is buggy. It's simply not guaranteed to work any more. O_DIRECT is kind of immaterial, except it's just one of those things where the atomic size is slightly more visible. But basically, packetized pipes with bigger packets than PIPE_BUF is random behavior. It may work. It may not. Thanks for your explanation. I am more curious about the user scene of this flag. @Li, so how to design this test? In this test, we don't have complex scene to test this automic unit. Best Regards Yang Xu Linus
Re: [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero
on 2019/07/25 11:10, Yang Xu wrote: --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2372,7 +2372,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = current->timer_slack_ns; break; case PR_SET_TIMERSLACK: -if (arg2<= 0) +if (arg2 == 0) current->timer_slack_ns = current->default_timer_slack_ns; A number of years ago Linus expressed approval of such comparisons with unsigned quantities. He felt that it improves readability a little - the reader doesn't have to scroll back and check the type. Hi Andrew It sounds good. ButWe still have to look at the actual situation. In here, this comparisons with unsigned quantities doesn't improvereadability. In turn, the code give user a wrongdescription as man page said " If arg2 is less than or equal to zero, the "current" timer slack is reset to the thread's default" timer slack value." If we set -1 in user space, we pass it into kernel as ULONG_MAX, it will not use default timer_slack value. Also, I guess that if value has no actual sense we can use this comparisons. In here, arg2 represents slack time. time will never less than 0. ps: whether we change or not change this comparisons, it doesn't affect logic. So if you think this patch is meaningless, I will accept it. Hi Andrew what do you think about it? update it or keep it. Thanks Yang Xu
Re: [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero
on 2019/07/25 10:14, Andrew Morton wrote: On Wed, 24 Jul 2019 10:11:48 +0800 Yang Xu wrote: Currently, when calling prctl(PR_SET_TIMERSLACK, arg2), arg2 is an unsigned long value, arg2 will never< 0. Negative judgment is meaningless, so remove it. ... --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2372,7 +2372,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = current->timer_slack_ns; break; case PR_SET_TIMERSLACK: - if (arg2<= 0) + if (arg2 == 0) current->timer_slack_ns = current->default_timer_slack_ns; A number of years ago Linus expressed approval of such comparisons with unsigned quantities. He felt that it improves readability a little - the reader doesn't have to scroll back and check the type. Hi Andrew It sounds good. ButWe still have to look at the actual situation. In here, this comparisons with unsigned quantities doesn't improvereadability. In turn, the code give user a wrongdescription as man page said " If arg2 is less than or equal to zero, the "current" timer slack is reset to the thread's default" timer slack value." If we set -1 in user space, we pass it into kernel as ULONG_MAX, it will not use default timer_slack value. Also, I guess that if value has no actual sense we can use this comparisons. In here, arg2 represents slack time. time will never less than 0. ps: whether we change or not change this comparisons, it doesn't affect logic. So if you think this patch is meaningless, I will accept it. Thanks Yang Xu
[PATCH v2] sys_prctl(): remove unsigned comparision with less than zero
Currently, when calling prctl(PR_SET_TIMERSLACK, arg2), arg2 is an
unsigned long value, arg2 will never < 0. Negative judgment is
meaningless, so remove it.
Fixes: 6976675d9404 ("hrtimer: create a "timer_slack" field in the task struct")
Signed-off-by: Yang Xu
Cc: Cyrill Gorcunov
---
kernel/sys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 2969304c29fe..701b5f00651d 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2372,7 +2372,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2,
unsigned long, arg3,
error = current->timer_slack_ns;
break;
case PR_SET_TIMERSLACK:
- if (arg2 <= 0)
+ if (arg2 == 0)
current->timer_slack_ns =
current->default_timer_slack_ns;
else
--
2.18.1
Re: [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK
on 2019/07/23 15:23, Cyrill Gorcunov wrote: On Tue, Jul 23, 2019 at 11:30:53AM +0800, Yang Xu wrote: arg2 will never< 0, for its type is 'unsigned long'. So negative judgment is meaningless. Signed-off-by: Yang Xu --- kernel/sys.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/sys.c b/kernel/sys.c index 2969304c29fe..399457d26bef 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2372,11 +2372,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = current->timer_slack_ns; break; case PR_SET_TIMERSLACK: - if (arg2<= 0) + if (arg2) + current->timer_slack_ns = arg2; + else current->timer_slack_ns = current->default_timer_slack_ns; - else - current->timer_slack_ns = arg2; break; case PR_MCE_KILL: if (arg4 | arg5) > From a glance it looks correct to me, but then... 1) you might simply compare with zero, iow if (arg2 == 0) instead of changing 7 lines Hi Cyril Indeed. simply compare with zero might be better. 2) according to man page passing negative value should be acceptable, though it never worked as expected. I've been grepping "git log" for this file and the former API is coming from commit 6976675d94042fbd446231d1bd8b7de71a980ada Author: Arjan van de Ven Date: Mon Sep 1 15:52:40 2008 -0700 hrtimer: create a "timer_slack" field in the task struct which is 11 years old by now. Nobody complained so far even when man page is saying pretty obviously PR_SET_TIMERSLACK (since Linux 2.6.28) Each thread has two associated timer slack values: a "default" value, and a "current" value. This operation sets the "current" timer slack value for the calling thread. If the nanosecond value supplied in arg2 is greater than zero, then the "current" value is set to this value. If arg2 is less than or equal to zero, the "current" timer slack is reset to the thread's "default" timer slack value. So i think to match the man page (and assuming that accepting negative value has been supposed) we should rather do if ((long)arg2< 0) Looks correct. But if we set a ULONG_MAX(PR_GET_TIMERSLACK also limits ULONG_MAX) value(about 4s) on 32bit machine, this code will think this value is a negative value and use default value. I guess man page was written as "less than or equal to zero" because of this confusing code(arg2<=0, but arg2 is an unsinged long value). I think we can change this man page and also add bounds value description. Also, I found a patch about arg2 is an unsigned long value commit 7fe5e04292e71af34ae171b88caa2a139e0b6125 Author: Chen Gang Date: Thu Feb 21 16:43:06 2013 -0800 sys_prctl(): arg2 is unsigned long which is never< 0 arg2 will never< 0, for its type is 'unsigned long' Also, use the provided macros. What do you think about it ? Thoughts?
[PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK
arg2 will never < 0, for its type is 'unsigned long'. So negative judgment is meaningless. Signed-off-by: Yang Xu --- kernel/sys.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/sys.c b/kernel/sys.c index 2969304c29fe..399457d26bef 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2372,11 +2372,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = current->timer_slack_ns; break; case PR_SET_TIMERSLACK: - if (arg2 <= 0) + if (arg2) + current->timer_slack_ns = arg2; + else current->timer_slack_ns = current->default_timer_slack_ns; - else - current->timer_slack_ns = arg2; break; case PR_MCE_KILL: if (arg4 | arg5) -- 2.18.1
