You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "GUIDINGLI (via GitHub)" <gi...@apache.org> on 2023/09/18 03:04:35 UTC
[GitHub] [nuttx] GUIDINGLI opened a new pull request, #10668: signal: use work_cancel_sync() to fix used after free
GUIDINGLI opened a new pull request, #10668:
URL: https://github.com/apache/nuttx/pull/10668
## Summary
wqueue: add work_cancel_sync() support
signal: use work_cancel_sync() to fix used after free
user thread: hpwork:
timer_create() with SIGEV_THREAD
timer_settime()
irq -> work_queue() add nxsig_notification_worker to Q
timer_delete()
nxsig_cancel_notification()
call nxsig_notification_worker()
work_cancel()
timer_free()
nxsig_notification_worker() used after free
root cause:
work_cancel() can't cancel work completely, the worker may alreay be running.
resolve:
use work_cancel_sync() API to cancel the work completely
## Impact
work queue
## Testing
BES board & SIM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] GUIDINGLI commented on a diff in pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "GUIDINGLI (via GitHub)" <gi...@apache.org>.
GUIDINGLI commented on code in PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#discussion_r1329539957
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -93,6 +96,21 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
work->worker = NULL;
ret = OK;
}
+ else if (nthread > 0)
+ {
+ int wndx;
+
+ for (wndx = 0; wndx < nthread; wndx++)
+ {
+ if (wqueue->worker[wndx].work == work &&
+ wqueue->worker[wndx].pid != _SCHED_GETTID())
Review Comment:
change to nx API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#discussion_r1328258414
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -93,6 +96,21 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
work->worker = NULL;
ret = OK;
}
+ else if (nthread > 0)
+ {
+ int wndx;
+
+ for (wndx = 0; wndx < nthread; wndx++)
+ {
+ if (wqueue->worker[wndx].work == work &&
+ wqueue->worker[wndx].pid != _SCHED_GETTID())
Review Comment:
Why _SCHED_GETTID macro is used? It is not a libc call, so we should call nx call directly here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#issuecomment-1722775673
Please rebase PR to fix CI failure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#discussion_r1328263358
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -93,6 +96,21 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
work->worker = NULL;
ret = OK;
}
+ else if (nthread > 0)
+ {
+ int wndx;
+
+ for (wndx = 0; wndx < nthread; wndx++)
+ {
+ if (wqueue->worker[wndx].work == work &&
+ wqueue->worker[wndx].pid != _SCHED_GETTID())
Review Comment:
since we want to avoid sched note syscall hook catch the kernel internal activities.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #10668:
URL: https://github.com/apache/nuttx/pull/10668
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#discussion_r1328520383
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -93,6 +96,21 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
work->worker = NULL;
ret = OK;
}
+ else if (nthread > 0)
+ {
+ int wndx;
+
+ for (wndx = 0; wndx < nthread; wndx++)
+ {
+ if (wqueue->worker[wndx].work == work &&
+ wqueue->worker[wndx].pid != _SCHED_GETTID())
Review Comment:
@pkarashchenko Please see this patch: https://github.com/apache/nuttx/pull/8394. There is the detailed information.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#discussion_r1328520383
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -93,6 +96,21 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
work->worker = NULL;
ret = OK;
}
+ else if (nthread > 0)
+ {
+ int wndx;
+
+ for (wndx = 0; wndx < nthread; wndx++)
+ {
+ if (wqueue->worker[wndx].work == work &&
+ wqueue->worker[wndx].pid != _SCHED_GETTID())
Review Comment:
Please see this patch: https://github.com/apache/nuttx/pull/8394
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#discussion_r1328523181
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -93,6 +96,21 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
work->worker = NULL;
ret = OK;
}
+ else if (nthread > 0)
+ {
+ int wndx;
+
+ for (wndx = 0; wndx < nthread; wndx++)
+ {
+ if (wqueue->worker[wndx].work == work &&
+ wqueue->worker[wndx].pid != _SCHED_GETTID())
Review Comment:
Yes, exactly. The `_SCHED_GETTID` is used in the places where code is shared between apps and kernel, like `libc`, so the question is do we compile code for kworker out of kernel?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #10668: signal: use work_cancel_sync() to fix used after free
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10668:
URL: https://github.com/apache/nuttx/pull/10668#discussion_r1328511059
##########
sched/wqueue/kwork_cancel.c:
##########
@@ -93,6 +96,21 @@ static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
work->worker = NULL;
ret = OK;
}
+ else if (nthread > 0)
+ {
+ int wndx;
+
+ for (wndx = 0; wndx < nthread; wndx++)
+ {
+ if (wqueue->worker[wndx].work == work &&
+ wqueue->worker[wndx].pid != _SCHED_GETTID())
Review Comment:
Still not clear why we can't use `nxsched_gettid()` here. Could you please provide more detailed description?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org