| Message ID | 20210924092111.20012-1-rpalethorpe@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] msgrcv07: Check negative msg type filters | expand |
Hi Richie,
LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
Hi Richie, ... > +++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c > @@ -232,6 +232,13 @@ static void test_negative_msgtyp(void) > "-msgtyp didn't get the first message in the queue with the lowest type"); > } > + TST_EXP_FAIL(msgrcv(queue_id, &rcv_buf, MSGSIZE, -MSGTYPE1, IPC_NOWAIT), > + ENOMSG, > + "-msgtype didn't recv next lowest msg"); > + TST_EXP_FAIL(msgrcv(queue_id, &rcv_buf, MSGSIZE, -MSGTYPE1, MSG_EXCEPT | IPC_NOWAIT), > + ENOMSG, > + "-msgtype (with except) didn't recv next lowest msg"); nit: I'd use msgtyp instead of msgtype (although it looks like typo, it's used without 'e' as a parametr name in man msgop(2) and also throughout this test (obviously can be changed during merge). Kind regards, Petr
Hello Petr, Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, > > ... >> +++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c >> @@ -232,6 +232,13 @@ static void test_negative_msgtyp(void) >> "-msgtyp didn't get the first message in the queue with the lowest type"); >> } > >> + TST_EXP_FAIL(msgrcv(queue_id, &rcv_buf, MSGSIZE, -MSGTYPE1, IPC_NOWAIT), >> + ENOMSG, >> + "-msgtype didn't recv next lowest msg"); >> + TST_EXP_FAIL(msgrcv(queue_id, &rcv_buf, MSGSIZE, -MSGTYPE1, MSG_EXCEPT | IPC_NOWAIT), >> + ENOMSG, >> + "-msgtype (with except) didn't recv next lowest msg"); > nit: I'd use msgtyp instead of msgtype (although it looks like typo, it's used > without 'e' as a parametr name in man msgop(2) and also throughout this test > (obviously can be changed during merge). Sure, sounds good. > > Kind regards, > Petr
Hi!
I guess that it would make more sense to add this to the msgrcv02.c as:
diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c
index cfb7d7446..b305d1f92 100644
--- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c
+++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c
@@ -21,6 +21,8 @@
* msgflg and no message of the requested type existed on the message queue.
*/
+#define _GNU_SOURCE
+
#include <string.h>
#include <sys/wait.h>
#include <sys/msg.h>
@@ -35,10 +37,12 @@ static int queue_id = -1;
static int bad_id = -1;
struct passwd *pw;
+#define MSGTYP 2
+
static struct buf {
long type;
char mtext[MSGSIZE];
-} rcv_buf, snd_buf = {MSGTYPE, "hello"};
+} rcv_buf, snd_buf = {MSGTYP, "hello"};
static struct tcase {
int *id;
@@ -49,12 +53,14 @@ static struct tcase {
int exp_user;
int exp_err;
} tcases[] = {
- {&queue_id, &rcv_buf, 4, 1, 0, 0, E2BIG},
- {&queue_id, &rcv_buf, MSGSIZE, 1, 0, 1, EACCES},
- {&queue_id, NULL, MSGSIZE, 1, 0, 0, EFAULT},
- {&bad_id, &rcv_buf, MSGSIZE, 1, 0, 0, EINVAL},
- {&queue_id, &rcv_buf, -1, 1, 0, 0, EINVAL},
- {&queue_id, &rcv_buf, MSGSIZE, 2, IPC_NOWAIT, 0, ENOMSG},
+ {&queue_id, &rcv_buf, 4, MSGTYP, 0, 0, E2BIG},
+ {&queue_id, &rcv_buf, MSGSIZE, MSGTYP, 0, 1, EACCES},
+ {&queue_id, NULL, MSGSIZE, MSGTYP, 0, 0, EFAULT},
+ {&bad_id, &rcv_buf, MSGSIZE, MSGTYP, 0, 0, EINVAL},
+ {&queue_id, &rcv_buf, -1, MSGTYP, 0, 0, EINVAL},
+ {&queue_id, &rcv_buf, MSGSIZE, MSGTYP+1, IPC_NOWAIT, 0, ENOMSG},
+ {&queue_id, &rcv_buf, MSGSIZE, -1, IPC_NOWAIT, 0, ENOMSG},
+ {&queue_id, &rcv_buf, MSGSIZE, -1, IPC_NOWAIT | MSG_EXCEPT, 0, ENOMSG},
};
What do you think?
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! > I guess that it would make more sense to add this to the msgrcv02.c as: > > diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c > index cfb7d7446..b305d1f92 100644 > --- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c > +++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv02.c > @@ -21,6 +21,8 @@ > * msgflg and no message of the requested type existed on the message queue. > */ > > +#define _GNU_SOURCE > + > #include <string.h> > #include <sys/wait.h> > #include <sys/msg.h> > @@ -35,10 +37,12 @@ static int queue_id = -1; > static int bad_id = -1; > struct passwd *pw; > > +#define MSGTYP 2 > + > static struct buf { > long type; > char mtext[MSGSIZE]; > -} rcv_buf, snd_buf = {MSGTYPE, "hello"}; > +} rcv_buf, snd_buf = {MSGTYP, "hello"}; > > static struct tcase { > int *id; > @@ -49,12 +53,14 @@ static struct tcase { > int exp_user; > int exp_err; > } tcases[] = { > - {&queue_id, &rcv_buf, 4, 1, 0, 0, E2BIG}, > - {&queue_id, &rcv_buf, MSGSIZE, 1, 0, 1, EACCES}, > - {&queue_id, NULL, MSGSIZE, 1, 0, 0, EFAULT}, > - {&bad_id, &rcv_buf, MSGSIZE, 1, 0, 0, EINVAL}, > - {&queue_id, &rcv_buf, -1, 1, 0, 0, EINVAL}, > - {&queue_id, &rcv_buf, MSGSIZE, 2, IPC_NOWAIT, 0, ENOMSG}, > + {&queue_id, &rcv_buf, 4, MSGTYP, 0, 0, E2BIG}, > + {&queue_id, &rcv_buf, MSGSIZE, MSGTYP, 0, 1, EACCES}, > + {&queue_id, NULL, MSGSIZE, MSGTYP, 0, 0, EFAULT}, > + {&bad_id, &rcv_buf, MSGSIZE, MSGTYP, 0, 0, EINVAL}, > + {&queue_id, &rcv_buf, -1, MSGTYP, 0, 0, EINVAL}, > + {&queue_id, &rcv_buf, MSGSIZE, MSGTYP+1, IPC_NOWAIT, 0, ENOMSG}, > + {&queue_id, &rcv_buf, MSGSIZE, -1, IPC_NOWAIT, 0, ENOMSG}, > + {&queue_id, &rcv_buf, MSGSIZE, -1, IPC_NOWAIT | MSG_EXCEPT, 0, ENOMSG}, > }; > > > What do you think? I don't know, the functionality in each test seems arbitrary. I would be happy with either patch.
Hi! > > What do you think? > > I don't know, the functionality in each test seems arbitrary. I would be > happy with either patch. I guess I slightly prefer having the negative tests in a single place...
Hi Richard > It should leave higher message types on the queue if they are above > the specified type's magnitude. > > Also check what happens when MSG_EXCEPT is passed with a negative msg > type. This behavior is unspecified, but presently the kernel ignores > the flag if the type is negative. The motivation to check this is long > handling in 32bit compat mode. If the msg type is not sign extended > then it will be treated as a large positive integer. In this case we > should receive the remaining message. > > On the current kernel under x86 the test passes because the system > call has an explicit compat variant which performs the sign > extension. Otherwise it would fail. > > Signed-off-by: Richard Palethorpe<rpalethorpe@suse.com> > --- > testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c > index 2c687c5c8..8635ef7a4 100644 > --- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c > +++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c > @@ -232,6 +232,13 @@ static void test_negative_msgtyp(void) > "-msgtyp didn't get the first message in the queue with the lowest type"); > } > > + TST_EXP_FAIL(msgrcv(queue_id,&rcv_buf, MSGSIZE, -MSGTYPE1, IPC_NOWAIT), > + ENOMSG, > + "-msgtype didn't recv next lowest msg"); > + TST_EXP_FAIL(msgrcv(queue_id,&rcv_buf, MSGSIZE, -MSGTYPE1, MSG_EXCEPT | IPC_NOWAIT), > + ENOMSG, > + "-msgtype (with except) didn't recv next lowest msg"); > + It should use TST_EXP_FAIL2 macro. Other than this, look good to me Reviewed-by: Yang Xu <xuyang2018.jy@fujitsu.com> Best Regards Yang Xu > exit: > SAFE_MSGCTL(queue_id, IPC_RMID, NULL); > queue_id = -1;
diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c index 2c687c5c8..8635ef7a4 100644 --- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c +++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c @@ -232,6 +232,13 @@ static void test_negative_msgtyp(void) "-msgtyp didn't get the first message in the queue with the lowest type"); } + TST_EXP_FAIL(msgrcv(queue_id, &rcv_buf, MSGSIZE, -MSGTYPE1, IPC_NOWAIT), + ENOMSG, + "-msgtype didn't recv next lowest msg"); + TST_EXP_FAIL(msgrcv(queue_id, &rcv_buf, MSGSIZE, -MSGTYPE1, MSG_EXCEPT | IPC_NOWAIT), + ENOMSG, + "-msgtype (with except) didn't recv next lowest msg"); + exit: SAFE_MSGCTL(queue_id, IPC_RMID, NULL); queue_id = -1;
It should leave higher message types on the queue if they are above the specified type's magnitude. Also check what happens when MSG_EXCEPT is passed with a negative msg type. This behavior is unspecified, but presently the kernel ignores the flag if the type is negative. The motivation to check this is long handling in 32bit compat mode. If the msg type is not sign extended then it will be treated as a large positive integer. In this case we should receive the remaining message. On the current kernel under x86 the test passes because the system call has an explicit compat variant which performs the sign extension. Otherwise it would fail. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- testcases/kernel/syscalls/ipc/msgrcv/msgrcv07.c | 7 +++++++ 1 file changed, 7 insertions(+)