[Do not merge]fs/mqueue/mq_check.c: group level error check for mq operations #6570

ritesh55555 · 2024-12-10T20:36:04Z

This commit creates an api mq_check() for mqueue module. This api will check the mq descriptor value in the mq des list of the calling task's group. This will help to remove unwanted error in mq_send and mq_receive apis.

sunghan-chang · 2024-12-11T03:37:04Z

os/kernel/mqueue/mq_receive.c

@@ -150,6 +150,11 @@ ssize_t mq_receive(mqd_t mqdes, FAR char *msg, size_t msglen, FAR int *prio)
 		return ERROR;
 	}

+	if (mq_check(mqdes) != OK) {
+		leave_cancellation_point();
+		return ERROR;


Before ERROR return, we should set proper errno.

* Return Value: * One success, the length of the selected message in bytes is returned. * On failure, -1 (ERROR) is returned and the errno is set appropriately: * * EAGAIN The queue was empty, and the O_NONBLOCK flag was set * for the message queue description referred to by 'mqdes'. * EPERM Message queue opened not opened for reading. * EMSGSIZE 'msglen' was less than the maxmsgsize attribute of the * message queue. * EINTR The call was interrupted by a signal handler. * EINVAL Invalid 'msg' or 'mqdes'

sunghan-chang · 2024-12-11T03:37:21Z

os/kernel/mqueue/mq_send.c

@@ -154,6 +154,11 @@ int mq_send(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio)
 		return ERROR;
 	}

+	if (mq_check(mqdes) != OK) {
+		leave_cancellation_point();
+		return ERROR;


Same as previous

sunghan-chang · 2024-12-11T06:09:43Z

@ritesh55555 @kishore-sn I thought when we close or unlink a mq, if we get wrong or invalid mqdes or mqname, it could corrupt mq inode. If we use invalid mqdes for mq send and receive, is it possible to be corrupted?

sunghan-chang · 2024-12-11T06:10:55Z

@ritesh55555 @kishore-sn I want to check whether this improves stability even it causes performance degradation by checking single queue every send and receive or not.

os/kernel/mqueue/mq_receive.c

os/fs/mqueue/mq_check.c

os/kernel/mqueue/mq_send.c

neel-samsung

Please check the github checks which are failing

kishore-sn · 2024-12-16T06:44:33Z

ue every send and receive

Hi @sunghan-chang,
Do you have any suggestion on how to check stability?
In my opinion, we can run all our test cases. But apart from that, I am not sure of any other method.

kishore-sn · 2024-12-16T07:06:56Z

os/kernel/mqueue/mq_getattr.c

@@ -105,6 +105,10 @@ int mq_getattr(mqd_t mqdes, struct mq_attr *mq_stat)
 {
 	int ret = ERROR;

+	if (mq_check(mqdes) != OK) {
+		return ERROR;


We need to set the errno before we return.

- This commit creates an api mq_check() for mqueue module. This api will check the mq descriptor value in the mq des list of the calling task's group. This will help to remove unwanted error in mq_send and mq_receive apis.

sunghan-chang · 2024-12-16T10:55:19Z

os/fs/mqueue/mq_check.c

+ *
+ ************************************************************************/
+
+int mq_check(mqd_t mqdes)


Let's rename the function to show "check what".

sunghan-chang · 2024-12-16T10:55:52Z

os/fs/mqueue/mq_check.c

+ *
+ * Return Value:
+ *   OK - if mqdes is present in the calling task group list of mqdes
+ *   ERROR - if mqdes is not present in the calling task group of mqdes


It does not return ERROR, it does EBADF.

sunghan-chang · 2024-12-16T10:56:23Z

os/include/mqueue.h

+/**
+ * @brief get message queue attributes
+ * @details @b #include <mqueue.h> \n
+ * POSIX API (refer to : http://pubs.opengroup.org/onlinepubs/9699919799/)


Is this the POSIX API?

sunghan-chang · 2024-12-16T10:58:01Z

os/include/mqueue.h

@@ -188,6 +188,13 @@ int mq_setattr(mqd_t mqdes, FAR const struct mq_attr *mq_stat, FAR struct mq_att
 * @since TizenRT v1.0
 */
 int mq_getattr(mqd_t mqdes, FAR struct mq_attr *mq_stat);
+/**
+ * @brief get message queue attributes


description looks wrong.

sunghan-chang · 2024-12-16T10:58:50Z

os/kernel/mqueue/mq_notify.c

@@ -161,6 +161,11 @@ int mq_notify(mqd_t mqdes, const struct sigevent *notification)
 		return ERROR;
 	}

+	if (mq_check(mqdes) != OK) {
+		set_errno(EBADF);


I think you should use the return value instead of hard coding here.

sunghan-chang · 2024-12-16T10:59:10Z

os/kernel/mqueue/mq_receive.c

@@ -145,6 +145,12 @@ ssize_t mq_receive(mqd_t mqdes, FAR char *msg, size_t msglen, FAR int *prio)
 	/* mq_receive() is a cancellation point */
 	(void)enter_cancellation_point();

+	if (mq_check(mqdes) != OK) {
+		leave_cancellation_point();
+		set_errno(EINVAL);


When it has EINVAL, when it has EBADF?

ritesh55555 · 2024-12-17T05:21:50Z

@ritesh55555 @kishore-sn I thought when we close or unlink a mq, if we get wrong or invalid mqdes or mqname, it could corrupt mq inode. If we use invalid mqdes for mq send and receive, is it possible to be corrupted?

i dont think the inode will get corrupted if we use invalid mqdes for mq send and receive . But will lead to unintended behavior

jeongarmy · 2024-12-19T02:39:52Z

os/fs/mqueue/mq_check.c

+	int ret = -EBADF;
+    	mqd_t mqdes_ptr;
+
+	FAR struct task_group_s *group = sched_self()->group;


I think we need to check mqdes and group because most of mq functions except mq_close don't check it.
DEBUGASSERT(mqdes != NULL && group != NULL);

jeongarmy · 2024-12-19T05:48:25Z

os/kernel/mqueue/mq_setattr.c

@@ -114,6 +114,10 @@ int mq_setattr(mqd_t mqdes, const struct mq_attr *mq_stat, struct mq_attr *oldst
 {
 	int ret = ERROR;

+	if (mq_check(mqdes) != OK) {
+		return ERROR;


Set errno is missnig

abhishek-samsung · 2024-12-24T02:13:01Z

os/include/mqueue.h

+ * POSIX API (refer to : http://pubs.opengroup.org/onlinepubs/9699919799/)
+ * @since TizenRT v1.0
+ */
+int mq_check(mqd_t mqdes);


This looks like an internal api for mqueue, we need not expose it to outsize. Please check if we can we move this to some internal header file.

abhishek-samsung · 2024-12-24T02:24:23Z

os/kernel/mqueue/mq_send.c

@@ -123,7 +123,8 @@
 *
 *   EAGAIN   The queue was empty, and the O_NONBLOCK flag was set for the
 *            message queue description referred to by mqdes.
- *   EINVAL   Either msg or mqdes is NULL or the value of prio is invalid.
+ *   EINVAL   Either msg or mqdes is NULL or the value of prio is invalid,
+ * 	 		  or mqdes is present in task group's mq list.


I think this should be,

Suggested change

* or mqdes is present in task group's mq list.

* or mqdes is not present in task group's mq list.

But here too, I think EBADF is better instead of EINVAL.

ritesh55555 force-pushed the mq_fix branch from 45a8b96 to a9138e8 Compare December 10, 2024 20:37

sunghan-chang reviewed Dec 11, 2024

View reviewed changes

kishore-sn reviewed Dec 11, 2024

View reviewed changes

os/kernel/mqueue/mq_receive.c Outdated Show resolved Hide resolved

os/fs/mqueue/mq_check.c Outdated Show resolved Hide resolved

os/kernel/mqueue/mq_send.c Outdated Show resolved Hide resolved

neel-samsung reviewed Dec 13, 2024

View reviewed changes

ritesh55555 force-pushed the mq_fix branch from a9138e8 to 9a71dec Compare December 16, 2024 06:40

ritesh55555 force-pushed the mq_fix branch 2 times, most recently from ae6edb3 to 3c40f54 Compare December 16, 2024 06:57

kishore-sn reviewed Dec 16, 2024

View reviewed changes

fs/mqueue/mq_check.c: group level error check for mq operations

93be26e

- This commit creates an api mq_check() for mqueue module. This api will check the mq descriptor value in the mq des list of the calling task's group. This will help to remove unwanted error in mq_send and mq_receive apis.

ritesh55555 force-pushed the mq_fix branch from 3c40f54 to 93be26e Compare December 16, 2024 08:32

sunghan-chang reviewed Dec 16, 2024

View reviewed changes

jeongarmy reviewed Dec 19, 2024

View reviewed changes

abhishek-samsung reviewed Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Do not merge]fs/mqueue/mq_check.c: group level error check for mq operations #6570

[Do not merge]fs/mqueue/mq_check.c: group level error check for mq operations #6570

ritesh55555 commented Dec 10, 2024

sunghan-chang Dec 11, 2024

sunghan-chang Dec 11, 2024

sunghan-chang commented Dec 11, 2024

sunghan-chang commented Dec 11, 2024

neel-samsung left a comment

kishore-sn commented Dec 16, 2024

kishore-sn Dec 16, 2024

sunghan-chang Dec 16, 2024

sunghan-chang Dec 16, 2024

sunghan-chang Dec 16, 2024

sunghan-chang Dec 16, 2024

sunghan-chang Dec 16, 2024

sunghan-chang Dec 16, 2024

ritesh55555 commented Dec 17, 2024

jeongarmy Dec 19, 2024 •

edited

Loading

jeongarmy Dec 19, 2024

abhishek-samsung Dec 24, 2024

abhishek-samsung Dec 24, 2024

	* or mqdes is present in task group's mq list.
	* or mqdes is not present in task group's mq list.

[Do not merge]fs/mqueue/mq_check.c: group level error check for mq operations #6570

Are you sure you want to change the base?

[Do not merge]fs/mqueue/mq_check.c: group level error check for mq operations #6570

Conversation

ritesh55555 commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang commented Dec 11, 2024

sunghan-chang commented Dec 11, 2024

neel-samsung left a comment

Choose a reason for hiding this comment

kishore-sn commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritesh55555 commented Dec 17, 2024

jeongarmy Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeongarmy Dec 19, 2024 •

edited

Loading