You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/11 16:29:48 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #5204: input/ajoystck: Always protect the au_open list by critical section
xiaoxiang781216 opened a new pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204
## Summary
it's wrong to protect by au_exclsem in some case and remove ao_closing
## Impact
## Testing
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r782415335
##########
File path: drivers/input/ajoystick.c
##########
@@ -331,10 +327,6 @@ static void ajoy_sample(FAR struct ajoy_upperhalf_s *priv)
}
}
- /* Enable/disable interrupt handling */
-
- ajoy_enable(priv);
Review comment:
Why `ajoy_enable` is not needed here?
##########
File path: drivers/input/ajoystick.c
##########
@@ -412,45 +407,14 @@ static int ajoy_close(FAR struct file *filep)
FAR struct ajoy_open_s *curr;
FAR struct ajoy_open_s *prev;
irqstate_t flags;
- bool closing;
- int ret;
DEBUGASSERT(filep && filep->f_priv && filep->f_inode);
opriv = filep->f_priv;
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
- /* Handle an improbable race conditions with the following atomic test
- * and set.
- *
- * This is actually a pretty feeble attempt to handle this. The
- * improbable race condition occurs if two different threads try to
- * close the joystick driver at the same time. The rule: don't do
- * that! It is feeble because we do not really enforce stale pointer
- * detection anyway.
- */
-
flags = enter_critical_section();
- closing = opriv->ao_closing;
- opriv->ao_closing = true;
- leave_critical_section(flags);
-
- if (closing)
- {
- /* Another thread is doing the close */
-
- return OK;
- }
-
- /* Get exclusive access to the driver structure */
Review comment:
I'm not sure in this fix. I would expect still to wait for `au_exclsem` in `close`. I'm expecting flow like:
```
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
ret = ajoy_takesem(&priv->au_exclsem);
if (ret < 0)
{
ierr("ERROR: ajoy_takesem failed: %d\n", ret);
return ret;
}
flags = enter_critical_section();
/* Find the open structure in the list of open structures for the device */
for (prev = NULL, curr = priv->au_open;
curr && curr != opriv;
prev = curr, curr = curr->ao_flink);
DEBUGASSERT(curr);
if (curr == NULL)
{
leave_critical_section(flags);
ierr("ERROR: Failed to find open entry\n");
ret = -ENOENT;
goto errout_with_exclsem;
}
/* Remove the structure from the device */
if (prev)
{
prev->ao_flink = opriv->ao_flink;
}
else
{
priv->au_open = opriv->ao_flink;
}
leave_critical_section(flags);
/* Cancel any pending notification */
nxsig_cancel_notification(&opriv->ao_work);
/* And free the open structure */
kmm_free(opriv);
/* Enable/disable interrupt handling */
ajoy_enable(priv);
return OK;
errout_with_exclsem:
ajoy_givesem(&priv->au_exclsem);
return ret;
```
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815344448
##########
File path: drivers/input/ajoystick.c
##########
@@ -331,10 +327,6 @@ static void ajoy_sample(FAR struct ajoy_upperhalf_s *priv)
}
}
- /* Enable/disable interrupt handling */
-
- ajoy_enable(priv);
Review comment:
since the status isn't changed
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could still keep a semaphore for `priv` protection and critical section to protect `opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to fix is that the original code protect the same resource some time with semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access ao_flink
so, the final result is that there is no protection at all. You can't protect the same resource by two semaphores or mix of semaphore and critical section. Since we need allow ajoy_interrupt is called from the interrupt context directly, the critical section is the only choice here, and then semaphore must be removed to avoid the potential misuse like the above in the future. Actually, I always prefer to use semaphore to protect resource, but has to keep the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is pending on `file_read` call? I briefly looked thru the code and see that seems like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while other file api is executing. This problem isn't specific to joystick/button drivers, it's a common issue to the whole file system, and can't handle inside the driver level. The old code try to fix this problem with ao_closing, but it impossible to work.
> Please add more reviewers to this change before merging it.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821627258
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
We can handle this by using a semaphore to protect `priv` and critical section to protect `opriv` list (please see https://github.com/apache/incubator-nuttx/pull/5204#discussion_r782490616).
Could you please write more about "All file handles(file system, char driver, block driver, socket) has the same issue" (some examples) so I can take a look?
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815466236
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
IMO the semaphore protects `priv` and critical section protects `opriv` + iteration thru the linked list
I expressed this in https://github.com/apache/incubator-nuttx/pull/5204#discussion_r782490616
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815394817
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Yes, al_sample is already called inside critical section before ajoy_sample:
https://github.com/apache/incubator-nuttx/pull/5204/files#diff-5803ad0a8a93175597d14948ffe9a46f4e1ad28b59d28bcb568f56553b1fa8f7R249-R256
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815394931
##########
File path: drivers/input/djoystick.c
##########
@@ -181,12 +162,6 @@ static void djoy_enable(FAR struct djoy_upperhalf_s *priv)
lower = priv->du_lower;
DEBUGASSERT(lower);
- /* This routine is called both task level and interrupt level, so
- * interrupts must be disabled.
- */
-
- flags = enter_critical_section();
Review comment:
Done.
--
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] [incubator-nuttx] pkarashchenko commented on pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#issuecomment-1053589448
Please fix
```
Error: input/djoystick.c:608:32: error: variable 'priv' set but not used [-Werror=unused-but-set-variable]
608 | FAR struct djoy_upperhalf_s *priv;
```
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820062588
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
al_sample is impossible to pend since it may be called inside interrupt handler. This is the root reason why we must protect it with critical section, not semaphore.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821687232
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
@xiaoxiang781216 thank you. Yes, the reuse of data by VFS is still an unhanded problem. Now I agree to keep your previous version
```
static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
size_t len)
{
FAR struct inode *inode;
FAR struct ajoy_open_s *opriv;
FAR struct ajoy_upperhalf_s *priv;
FAR const struct ajoy_lowerhalf_s *lower;
int ret;
DEBUGASSERT(filep && filep->f_inode);
opriv = filep->f_priv;
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
/* Make sure that the buffer is sufficiently large to hold at least one
* complete sample.
*
* REVISIT: Should also check buffer alignment.
*/
if (len < sizeof(struct ajoy_sample_s))
{
ierr("ERROR: buffer too small: %lu\n", (unsigned long)len);
return -EINVAL;
}
/* Get exclusive access to the driver structure */
flags = enter_critical_section();
/* Read and return the current state of the joystick buttons */
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
if (ret >= 0)
{
opriv->ao_pollpending = false;
ret = sizeof(struct ajoy_sample_s);
}
leave_critical_section(flags);
return (ssize_t)ret;
}
```
and resolve a limitation of a driver based on VFS device in a different way. This will be a limitation for now.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821870255
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Done.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815352567
##########
File path: drivers/input/ajoystick.c
##########
@@ -412,45 +407,14 @@ static int ajoy_close(FAR struct file *filep)
FAR struct ajoy_open_s *curr;
FAR struct ajoy_open_s *prev;
irqstate_t flags;
- bool closing;
- int ret;
DEBUGASSERT(filep && filep->f_priv && filep->f_inode);
opriv = filep->f_priv;
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
- /* Handle an improbable race conditions with the following atomic test
- * and set.
- *
- * This is actually a pretty feeble attempt to handle this. The
- * improbable race condition occurs if two different threads try to
- * close the joystick driver at the same time. The rule: don't do
- * that! It is feeble because we do not really enforce stale pointer
- * detection anyway.
- */
-
flags = enter_critical_section();
- closing = opriv->ao_closing;
- opriv->ao_closing = true;
- leave_critical_section(flags);
-
- if (closing)
- {
- /* Another thread is doing the close */
-
- return OK;
- }
-
- /* Get exclusive access to the driver structure */
Review comment:
It's enough to protect the shared resource by one method. Since it's very confuse and error prone to protect by both semaphore and critical section, I remove au_exclsem from ajoy_upperhalf_s.
--
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] [incubator-nuttx] xiaoxiang781216 commented on pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#issuecomment-1052491471
> I found a similar issue in `drivers/input/button_upper.c`
Yes, I will fix button_upper too.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815356949
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Is `lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);` -> `al_sample` implementation ready that now it is called from inside the critical section?
##########
File path: drivers/input/ajoystick.c
##########
@@ -181,12 +162,6 @@ static void ajoy_enable(FAR struct ajoy_upperhalf_s *priv)
lower = priv->au_lower;
DEBUGASSERT(lower);
- /* This routine is called both task level and interrupt level, so
- * interrupts must be disabled.
- */
-
- flags = enter_critical_section();
Review comment:
Probably need to remove `irqstate_t flags;` as well
##########
File path: drivers/input/ajoystick.c
##########
@@ -686,7 +612,8 @@ static int ajoy_poll(FAR struct file *filep, FAR struct pollfd *fds,
FAR struct inode *inode;
FAR struct ajoy_upperhalf_s *priv;
FAR struct ajoy_open_s *opriv;
- int ret;
+ irqstate_t flags;
+ int ret = 0;
Review comment:
```suggestion
int ret = OK;
```
##########
File path: drivers/input/djoystick.c
##########
@@ -524,12 +459,7 @@ static ssize_t djoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = djoy_takesem(&priv->du_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: djoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Is `dl_sample` implementation ready that now it is called from inside the critical section?
##########
File path: drivers/input/djoystick.c
##########
@@ -181,12 +162,6 @@ static void djoy_enable(FAR struct djoy_upperhalf_s *priv)
lower = priv->du_lower;
DEBUGASSERT(lower);
- /* This routine is called both task level and interrupt level, so
- * interrupts must be disabled.
- */
-
- flags = enter_critical_section();
Review comment:
Probably need to remove `irqstate_t flags;` as well
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821592496
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
In this case, opriv is already freed by ajoy_close, how to detect it with the freed memory? All file handles(file system, char driver, block driver, socket) has the same issue, as I said before it need handle in the general way.
Before that, it's user's responsibility not to call close if any FS API is on going.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821660668
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
So what is not handled in this case?
1. Thread A just enter ajoy_close and switch to thread B. -- I assume that this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the `ajoy_takesem(&priv->au_exclsem);` call.
2. Thread B just enter ajoy_read and switch to thread A again. -- This will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside the `ret = ajoy_takesem(&priv->au_exclsem);` call.
3. Thread A continue ajoy_close and release opriv -- Yes, so in this case we need to access `filep->f_priv` (`opriv`) after `ret = ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be set to `NULL` after memory is freed.
4. Thread B resume and touch the freed memory -- this will not happen if `ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is accessed after that.
My assumption is next: The `priv->au_exclsem` protects any modification of `node->i_private` data and `filep->f_priv` data. If there is no modification, then we do not need to take `priv->au_exclsem`. We can try to leverage critical section approach.
In other words the `ajoy_read` should look like:
```
static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
size_t len)
{
FAR struct inode *inode;
FAR struct ajoy_open_s *opriv;
FAR struct ajoy_upperhalf_s *priv;
FAR const struct ajoy_lowerhalf_s *lower;
irqstate_t flags;
int ret;
DEBUGASSERT(filep && filep->f_inode);
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
/* Make sure that the buffer is sufficiently large to hold at least one
* complete sample.
*
* REVISIT: Should also check buffer alignment.
*/
if (len < sizeof(struct ajoy_sample_s))
{
ierr("ERROR: buffer too small: %lu\n", (unsigned long)len);
return -EINVAL;
}
/* Read and return the current state of the joystick buttons */
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
if (ret >= 0)
{
/* Get exclusive access to the driver instance structure */
flags = enter_critical_section();
opriv = filep->f_priv;
if (opriv != NULL)
{
opriv->ao_pollpending = false;
ret = sizeof(struct ajoy_sample_s);
}
else
{
ret = -EPIPE;
}
leave_critical_section(flags);
}
return (ssize_t)ret;
}
```
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821453974
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
The situation that I described is still not handled. We need to check if driver is still opened after `lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);` returns and if we can execute `opriv->ao_pollpending = false;`.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821869783
##########
File path: drivers/input/ajoystick.c
##########
@@ -380,14 +344,12 @@ static int ajoy_open(FAR struct file *filep)
ajoy_enable(priv);
+ leave_critical_section(flags);
Review comment:
Done.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821871849
##########
File path: drivers/input/ajoystick.c
##########
@@ -467,22 +398,20 @@ static int ajoy_close(FAR struct file *filep)
priv->au_open = opriv->ao_flink;
}
+ /* Enable/disable interrupt handling */
+
+ ajoy_enable(priv);
+
+ leave_critical_section(flags);
Review comment:
It don't need since nobody should touch opriv once ajoy_close is enter, otherwise we can't fix the race condition as discuss above.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r819914782
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
The current PR does not reflect the actual code and should be rebased, so I will write a comment taking into assumption that PR is based on the latest mainline.
I'm not sure if what I describe is a valid use case, so please correct me if I'm wrong.
On the latest mainline the next code is present:
```
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
if (ret >= 0)
{
opriv->ao_pollpending = false;
ret = sizeof(struct ajoy_sample_s);
}
```
So I'm thinking if that can be a situation when we enter a critical section and then `al_sample` pends. While we are waiting in `al_sample` the `ajoy_close` is called and that leads to `kmm_free(opriv);` so after we return from `al_sample` the `opriv->ao_pollpending = false;` can access released memory.
In general I do not understand what we protect here with a critical section? Maybe critical section can be removed at all from `ajoy_read`?
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821674599
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> So what is not handled in this case?
>
> 1. Thread A just enter ajoy_close and switch to thread B. -- I assume that this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the `ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem.
> 2. Thread B just enter ajoy_read and switch to thread A again. -- This will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside the `ret = ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem too.
> 3. Thread A continue ajoy_close and release opriv -- Yes, so in this case we need to access `filep->f_priv` (`opriv`) after `ret = ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be set to `NULL` after memory is freed.
Yes.
> 4. Thread B resume and touch the freed memory -- this will not happen if `ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is accessed after that.
>
Once ajoy_close return at the step 3, filep may be reused by VFS layer and then f_priv may point to a different struct, which make the check of filep->f_priv useless.
> My assumption is next: The `priv->au_exclsem` protects any modification of `node->i_private` data and `filep->f_priv` data. If there is no modification, then we do not need to take `priv->au_exclsem`. We can try to leverage critical section approach. In other words the `ajoy_read` should look like:
>
> ```
> /* Get exclusive access to the driver structure */
>
> if (ret >= 0)
> {
> flags = enter_critical_section();
>
> DEBUGASSERT(filep && filep->f_inode);
> opriv = filep->f_priv;
>
> if (opriv != NULL)
> {
> opriv->ao_pollpending = false;
> ret = sizeof(struct ajoy_sample_s);
> }
> else
> {
> ret = -EPIPE;
> }
>
> leave_critical_section(flags);
> }
>
> return (ssize_t)ret;
> }
> ```
neither semaphore nor critical section can resolve the above sequence.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821720907
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
No problem, I enjoy very much this kind of technical conversation.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821634793
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
You can't fix the race condition in all case. How to handle this sequence?
1. Thread A just enter ajoy_close and switch to thread B
2. Thread B just enter ajoy_read and switch to thread A again
3. Thread A continue ajoy_close and release opriv
4. Thread B resume and touch the freed memory
Basically, the resource release is very very special, you can't protect the resource by the normal approach.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815457612
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
vfs layer can't handle this case correctly in the complex case. To avoid the used after free, the better solution is:
1. Add a reference count to struct file
2. Initialize the reference count to 1 in file_open
3. Decrease the reference count in file_close
4. Increase count before call file_xxx and decrease after
5. Release the resource only when the reference count become zero
This way, the real release happen(delayed) after file_read return when file_close is called during file_read is executing.
The similar approach is used in Linux. Before this infrastructure get implemented, I would suggest that the caller should never call close if any other file related api is executing.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815394964
##########
File path: drivers/input/ajoystick.c
##########
@@ -181,12 +162,6 @@ static void ajoy_enable(FAR struct ajoy_upperhalf_s *priv)
lower = priv->au_lower;
DEBUGASSERT(lower);
- /* This routine is called both task level and interrupt level, so
- * interrupts must be disabled.
- */
-
- flags = enter_critical_section();
Review comment:
Done.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815395056
##########
File path: drivers/input/ajoystick.c
##########
@@ -686,7 +612,8 @@ static int ajoy_poll(FAR struct file *filep, FAR struct pollfd *fds,
FAR struct inode *inode;
FAR struct ajoy_upperhalf_s *priv;
FAR struct ajoy_open_s *opriv;
- int ret;
+ irqstate_t flags;
+ int ret = 0;
Review comment:
Done.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815455016
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
It's fine to call file_read inside critical section, since the implementation of critical section in nuttx allow the thread enter the wait state while the critical section is hold.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815460851
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Yes. The real use case would require reference counter. Or we could still keep a semaphore for `priv` protection and critical section to protect `opriv` manipulations.
I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is pending on `file_read` call? I briefly looked thru the code and see that seems like nothing harmful, but I still have some doubts.
Please add more reviewers to this change before merging it.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820247867
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
No. And that is why I'm keeping this discussion open. `al_buttons` access only board GPIOs via arch specific calls and do not use pending primitives (example sama5/giant-board: `ajoy_buttons` calls `sam_pioread`), while `al_sample` can be implemented as a reader from a file device and can pend (example sama5/giant-board: `ajoy_sample` calls `file_read` and `ajoy_buttons` at the end).
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820068541
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
`lower->al_sample` is not called inside interrupt handler. At least what I see from the code now.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821688179
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
@xiaoxiang781216 thank you for your patience in this discussion.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821674599
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> So what is not handled in this case?
>
> 1. Thread A just enter ajoy_close and switch to thread B. -- I assume that this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the `ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem.
> 2. Thread B just enter ajoy_read and switch to thread A again. -- This will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside the `ret = ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem too.
> 3. Thread A continue ajoy_close and release opriv -- Yes, so in this case we need to access `filep->f_priv` (`opriv`) after `ret = ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be set to `NULL` after memory is freed.
Yes.
> 4. Thread B resume and touch the freed memory -- this will not happen if `ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is accessed after that.
>
Once ajoy_close return at the step 3, filep may be reused by VFS layer and then f_priv may point to a different struct, which make the check of filep->f_priv useless.
> My assumption is next: The `priv->au_exclsem` protects any modification of `node->i_private` data and `filep->f_priv` data. If there is no modification, then we do not need to take `priv->au_exclsem`. We can try to leverage critical section approach. In other words the `ajoy_read` should look like:
>
> ```
> /* Get exclusive access to the driver structure */
>
> if (ret >= 0)
> {
> flags = enter_critical_section();
>
> DEBUGASSERT(filep && filep->f_inode);
> opriv = filep->f_priv;
>
> if (opriv != NULL)
> {
> opriv->ao_pollpending = false;
> ret = sizeof(struct ajoy_sample_s);
> }
> else
> {
> ret = -EPIPE;
> }
>
> leave_critical_section(flags);
> }
>
> return (ssize_t)ret;
> }
> ```
neither semaphore nor critical section can resolve the above sequence.
To handle the race condition between destruct and other paired functions, the best approach is the reference count and delay release, other methods may look like work, but not work really.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821660668
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
So what is not handled in this case?
1. Thread A just enter ajoy_close and switch to thread B. -- I assume that this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the `ajoy_takesem(&priv->au_exclsem);` call.
2. Thread B just enter ajoy_read and switch to thread A again. -- This will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside the `ret = ajoy_takesem(&priv->au_exclsem);` call.
3. Thread A continue ajoy_close and release opriv -- Yes, so in this case we need to access `filep->f_priv` (`opriv`) after `ret = ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be set to `NULL` after memory is freed.
4. Thread B resume and touch the freed memory -- this will not happen if `ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is accessed after that.
My assumption is next: The `priv->au_exclsem` protects any modification of `node->i_private` data and `filep->f_priv` data. If there is no modification, then we do not need to take `priv->au_exclsem`. We can try to leverage critical section approach.
In other words the `ajoy_read` should look like:
```
static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
size_t len)
{
FAR struct inode *inode;
FAR struct ajoy_open_s *opriv;
FAR struct ajoy_upperhalf_s *priv;
FAR const struct ajoy_lowerhalf_s *lower;
irqstate_t flags;
int ret;
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
/* Make sure that the buffer is sufficiently large to hold at least one
* complete sample.
*
* REVISIT: Should also check buffer alignment.
*/
if (len < sizeof(struct ajoy_sample_s))
{
ierr("ERROR: buffer too small: %lu\n", (unsigned long)len);
return -EINVAL;
}
/* Read and return the current state of the joystick buttons */
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
/* Get exclusive access to the driver structure */
if (ret >= 0)
{
flags = enter_critical_section();
DEBUGASSERT(filep && filep->f_inode);
opriv = filep->f_priv;
if (opriv != NULL)
{
opriv->ao_pollpending = false;
ret = sizeof(struct ajoy_sample_s);
}
else
{
ret = -EPIPE;
}
leave_critical_section(flags);
}
return (ssize_t)ret;
}
```
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821627258
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
We can handle this by using a semaphore to protect `priv` and critical section to protect `opriv` list.
Could you please write more about "All file handles(file system, char driver, block driver, socket) has the same issue" (some examples) so I can take a look?
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815452990
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Sorry I do not see that `al_sample` is called from `ajoy_sample`
--
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] [incubator-nuttx] xiaoxiang781216 commented on pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#issuecomment-1053591488
> Please fix
>
> ```
> Error: input/djoystick.c:608:32: error: variable 'priv' set but not used [-Werror=unused-but-set-variable]
> 608 | FAR struct djoy_upperhalf_s *priv;
> ```
Done.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815457612
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
vfs layer can't handle this case correctly in the complex case. To avoid the used after free, the better solution is:
1. Add a reference count to struct file
2. Initialize the reference count to 1 in file_open
3. Decrease the reference count in file_close
4. Increase count before call file_xxx and decrease after
5. Release the resource only when the reference count become zero
This way, the real release happen(delayed) after file_read return when file_close is called during file_read is executing.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815456175
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
It will work only because we do not have any manipulations with `priv` after `al_sample` call. Otherwise in theory if we enter a critical section and then pend inside the `file_read` call and during that time `ajoy_close` is called, then after returning from `file_read` the `priv` would point to already free-ed memory.
But this is not a theoretical case, so I'm fine with this change even if I do not like cases like this.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could still keep a semaphore for `priv` protection and critical section to protect `opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to fix is that the original code protect the same resource some time with semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access ao_flink
so, the final result is that there is no protection at all. You can't protect the same resource by two semaphores or mix of semaphore and critical section. Since we need allow ajoy_interrupt is called from the interrupt context directly, the critical section is the only choice here, and then semaphore must be removed to avoid the potential misuse like the above in the future. Actually, I always prefer to use semaphore to protect resource, but has to keep the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is pending on `file_read` call? I briefly looked thru the code and see that seems like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while other file api is executing. This problem isn't specific to joystick/button drivers, it's a common issue to the whole file system, and can't handle inside the driver level. The old code try to fix this problem with ao_closing, but it's impossible to work after the simple analysis.
> Please add more reviewers to this change before merging it.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820208640
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
ajoy_interrupt will be called inside interrupt handler which call lower->al_sample through ajoy_sample indirectly. If ajoy_interrupt is called only in thread context. All protection can be switch to the semaphore instead in first place.
--
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] [incubator-nuttx] xiaoxiang781216 commented on pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#issuecomment-1060136591
Done.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r819810584
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> Probably you are right, but the usage of semaphore gives us exclusive access with waiting for a task context while critical section resolves mutual exclusion between task and interrupt context inside the driver. I mean that only one of multiple callers of `ajoy_read` will call `lower->al_sample()`+return from `al_sample` with semaphore and multiple callers will call `lower->al_sample()` in case if critical section is used and `al_sample()` call a pending primitive. That is conceptually difference that I'm trying to clarify the behavior.
You mean al_sample may switch out inside critical section? Yes, if it wake up a high priority thread.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could still keep a semaphore for `priv` protection and critical section to protect `opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to fix is that the original code protect the same resource some time with semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access ao_flink
so, the final result is that there is no protection at all. You can't protect the same resource by two semaphores or mi of semaphore and critical section. Since we need allow ajoy_interrupt is called from the interrupt context directly, the critical section is the only choice, and then semaphore must be removed to avoid the potential misuse like the above in the future. Actually, I always prefer to use semaphore to protect resource, but has to keep the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is pending on `file_read` call? I briefly looked thru the code and see that seems like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while other file api is executing. This problem isn't specific to joystick/button drivers, it's a common issue to the whole file system, and can't handle inside the driver level. The old code try to fix this problem with ao_closing, but it impossible to work.
> Please add more reviewers to this change before merging it.
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could still keep a semaphore for `priv` protection and critical section to protect `opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to fix is that the original code protect the same resource some time with semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access ao_flink
so, the final result is that there is no protection at all. You can't protect the same resource by two semaphores or mix of semaphore and critical section. Since we need allow ajoy_interrupt is called from the interrupt context directly, the critical section is the only choice, and then semaphore must be removed to avoid the potential misuse like the above in the future. Actually, I always prefer to use semaphore to protect resource, but has to keep the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is pending on `file_read` call? I briefly looked thru the code and see that seems like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while other file api is executing. This problem isn't specific to joystick/button drivers, it's a common issue to the whole file system, and can't handle inside the driver level. The old code try to fix this problem with ao_closing, but it impossible to work.
> Please add more reviewers to this change before merging it.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could still keep a semaphore for `priv` protection and critical section to protect `opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to fix is that the original code protect the same resource some time with semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access ao_flink
so, the final result is that no protection at all. You can't protect the same resource by two semaphores or mi of semaphore and critical section. Since we need allow ajoy_interrupt is called from the interrupt context directly, the critical section is the only choice, and then semaphore must be removed to avoid the potential misuse like the above in the future. Actually, I always prefer to use semaphore to protect resource, but has to keep the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is pending on `file_read` call? I briefly looked thru the code and see that seems like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while other file api is executing. This problem isn't specific to joystick/button drivers, it's a common issue to the whole file system, and can't handle inside the driver level. The old code try to fix this problem with ao_closing, but it impossible to work.
> Please add more reviewers to this change before merging it.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821660668
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
So what is not handled in this case?
1. Thread A just enter ajoy_close and switch to thread B. -- I assume that this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the `ajoy_takesem(&priv->au_exclsem);` call.
2. Thread B just enter ajoy_read and switch to thread A again. -- This will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside the `ret = ajoy_takesem(&priv->au_exclsem);` call.
3. Thread A continue ajoy_close and release opriv -- Yes, so in this case we need to access `filep->f_priv` (`opriv`) after `ret = ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be set to `NULL` after memory is freed.
4. Thread B resume and touch the freed memory -- this will not happen if `ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is accessed after that.
My assumption is next: The `priv->au_exclsem` protects any modification of `node->i_private` data and `filep->f_priv` data. If there is no modification, then we do not need to take `priv->au_exclsem`. We can try to leverage critical section approach.
In other words the `ajoy_read` should look like:
```
static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
size_t len)
{
FAR struct inode *inode;
FAR struct ajoy_open_s *opriv;
FAR struct ajoy_upperhalf_s *priv;
FAR const struct ajoy_lowerhalf_s *lower;
irqstate_t flags;
int ret;
DEBUGASSERT(filep && filep->f_inode);
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
/* Make sure that the buffer is sufficiently large to hold at least one
* complete sample.
*
* REVISIT: Should also check buffer alignment.
*/
if (len < sizeof(struct ajoy_sample_s))
{
ierr("ERROR: buffer too small: %lu\n", (unsigned long)len);
return -EINVAL;
}
/* Read and return the current state of the joystick buttons */
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
if (ret >= 0)
{
/* Get exclusive access to the driver instance structure */
flags = enter_critical_section();
opriv = filep->f_priv;
if (opriv != NULL)
{
opriv->ao_pollpending = false;
ret = sizeof(struct ajoy_sample_s);
}
else
{
ret = -EPIPE;
}
leave_critical_section(flags);
}
return (ssize_t)ret;
}
```
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821592496
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
In this case, opriv is already freed by ajoy_close, how to detect it with the freed memory? All file handles(file system, char driver, block driver, socket) has the same issue, as I said before it need handle in the general way.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r822061271
##########
File path: drivers/input/button_upper.c
##########
@@ -887,7 +801,6 @@ int btn_register(FAR const char *devname,
return OK;
errout_with_priv:
Review comment:
I think we can remove `errout_with_priv` here and just `kmm_free(priv);` in `if (ret < 0)` instead.
##########
File path: drivers/input/ajoystick.c
##########
@@ -835,7 +748,6 @@ int ajoy_register(FAR const char *devname,
return OK;
errout_with_priv:
Review comment:
I think we can remove `errout_with_priv` here and just `kmm_free(priv);` in `if (ret < 0)` instead.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821687783
##########
File path: drivers/input/ajoystick.c
##########
@@ -380,14 +344,12 @@ static int ajoy_open(FAR struct file *filep)
ajoy_enable(priv);
+ leave_critical_section(flags);
Review comment:
Let's move this after `filep->f_priv = (FAR void *)opriv;`
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821674599
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> So what is not handled in this case?
>
> 1. Thread A just enter ajoy_close and switch to thread B. -- I assume that this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the `ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem.
> 2. Thread B just enter ajoy_read and switch to thread A again. -- This will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside the `ret = ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem too.
> 3. Thread A continue ajoy_close and release opriv -- Yes, so in this case we need to access `filep->f_priv` (`opriv`) after `ret = ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be set to `NULL` after memory is freed.
Yes.
> 4. Thread B resume and touch the freed memory -- this will not happen if `ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is accessed after that.
>
Once ajoy_close return at the step 3, filep may be reused by VFS layer and then f_priv may point to a different struct, which make the check of filep->f_priv useless.
> My assumption is next: The `priv->au_exclsem` protects any modification of `node->i_private` data and `filep->f_priv` data. If there is no modification, then we do not need to take `priv->au_exclsem`. We can try to leverage critical section approach. In other words the `ajoy_read` should look like:
>
> ```
> /* Get exclusive access to the driver structure */
>
> if (ret >= 0)
> {
> flags = enter_critical_section();
>
> DEBUGASSERT(filep && filep->f_inode);
> opriv = filep->f_priv;
>
> if (opriv != NULL)
> {
> opriv->ao_pollpending = false;
> ret = sizeof(struct ajoy_sample_s);
> }
> else
> {
> ret = -EPIPE;
> }
>
> leave_critical_section(flags);
> }
>
> return (ssize_t)ret;
> }
> ```
neither semaphore nor critical section can resolve the above sequency.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r822280662
##########
File path: drivers/input/button_upper.c
##########
@@ -887,7 +801,6 @@ int btn_register(FAR const char *devname,
return OK;
errout_with_priv:
Review comment:
Done.
##########
File path: drivers/input/ajoystick.c
##########
@@ -835,7 +748,6 @@ int ajoy_register(FAR const char *devname,
return OK;
errout_with_priv:
Review comment:
Done.
--
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] [incubator-nuttx] pkarashchenko merged pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820208640
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
ajoy_interrupt will be called inside interrupt handler which call lower->al_sample through ajoy_sample indirectly. If ajoy_interrupt is called only in thread context. All protection can be switch to the semaphore instead.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815467176
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
The link list is just a demo, if you evaluate each field on by one, you will find that all most fields touch in both the thread context and interrupt context.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815468410
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Probably you are right, but the usage of semaphore gives us exclusive access with waiting for a task context while critical section resolves mutual exclusion between task and interrupt context inside the driver. I mean that only one of multiple callers of `ajoy_read` will call `lower->al_sample()`+return from `al_sample` with semaphore and multiple callers will call `lower->al_sample()` in case if critical section is used and `al_sample()` call a pending primitive. That is conceptually difference that I'm trying to clarify the behavior.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815394775
##########
File path: drivers/input/djoystick.c
##########
@@ -524,12 +459,7 @@ static ssize_t djoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = djoy_takesem(&priv->du_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: djoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Yes, since dl_sample is already be called inside the critical secition in djoy_sample:
https://github.com/apache/incubator-nuttx/pull/5204/files#diff-cb78728fc9ab1e7ff74388d2382edcc0701a691fa0d737dcffce197a2eb8f8baR249-R254
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820243922
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Oh, yes, but ajoy_sample call lower->al_buttons:
https://github.com/apache/incubator-nuttx/pull/5204/files#diff-5803ad0a8a93175597d14948ffe9a46f4e1ad28b59d28bcb568f56553b1fa8f7R241
I would expect that al_sample has the similar implementation requirement as al_buttons. Do you think the assumption reasonable?
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820227187
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Sorry, but `ajoy_interrupt` calls `ajoy_sample`, but not `lower->al_sample`. The `lower->al_sample` is called only from `ajoy_read`. So interrupt handles only a change of buttons and wake-up reader and then reader reads the device data.
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820249165
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
Ok, I move enter_critical_section after call low->al_sample.
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820295759
##########
File path: drivers/input/ajoystick.c
##########
@@ -516,27 +446,22 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
return -EINVAL;
}
- /* Get exclusive access to the driver structure */
-
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
-
/* Read and return the current state of the joystick buttons */
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
+
+ /* Get exclusive access to the driver structure */
+
+ flags = enter_critical_section();
if (ret >= 0)
{
opriv->ao_pollpending = false;
ret = sizeof(struct ajoy_sample_s);
}
- ajoy_givesem(&priv->au_exclsem);
+ leave_critical_section(flags);
Review comment:
```suggestion
if (ret >= 0)
{
flags = enter_critical_section();
opriv->ao_pollpending = false;
leave_critical_section(flags);
ret = sizeof(struct ajoy_sample_s);
}
```
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821661720
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
And `ajoy_close` definitely be modified to have
```
kmm_free(opriv);
filep->f_priv = NULL;
```
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821660668
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
So what is not handled in this case?
1. Thread A just enter ajoy_close and switch to thread B. -- I assume that this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the `ajoy_takesem(&priv->au_exclsem);` call.
2. Thread B just enter ajoy_read and switch to thread A again. -- This will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside the `ret = ajoy_takesem(&priv->au_exclsem);` call.
3. Thread A continue ajoy_close and release opriv -- Yes, so in this case we need to access `filep->f_priv` (`opriv`) after `ret = ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be set to `NULL` after memory is freed.
4. Thread B resume and touch the freed memory -- this will not happen if `ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is accessed after that.
My assumption is next: The `priv->au_exclsem` protects any modification of `node->i_private` data and `filep->f_priv` data. If there is no modification, then we do not need to take `priv->au_exclsem`. We can try to leverage critical section approach.
In other words the `ajoy_read` should look like:
```
static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
size_t len)
{
FAR struct inode *inode;
FAR struct ajoy_open_s *opriv;
FAR struct ajoy_upperhalf_s *priv;
FAR const struct ajoy_lowerhalf_s *lower;
irqstate_t flags;
int ret;
DEBUGASSERT(filep && filep->f_inode);
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
/* Make sure that the buffer is sufficiently large to hold at least one
* complete sample.
*
* REVISIT: Should also check buffer alignment.
*/
if (len < sizeof(struct ajoy_sample_s))
{
ierr("ERROR: buffer too small: %lu\n", (unsigned long)len);
return -EINVAL;
}
/* Read and return the current state of the joystick buttons */
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
/* Get exclusive access to the driver structure */
if (ret >= 0)
{
flags = enter_critical_section();
opriv = filep->f_priv;
if (opriv != NULL)
{
opriv->ao_pollpending = false;
ret = sizeof(struct ajoy_sample_s);
}
else
{
ret = -EPIPE;
}
leave_critical_section(flags);
}
return (ssize_t)ret;
}
```
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821690573
##########
File path: drivers/input/ajoystick.c
##########
@@ -467,22 +398,20 @@ static int ajoy_close(FAR struct file *filep)
priv->au_open = opriv->ao_flink;
}
+ /* Enable/disable interrupt handling */
+
+ ajoy_enable(priv);
+
+ leave_critical_section(flags);
Review comment:
Let's move this after `kmm_free(opriv);` call
--
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] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821634793
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
You can't fix the race condition in all case. How to handle this sequence?
1. Thread A just enter ajoy_close and switch to thread B
2. Thread B just enter ajoy_read and switch to thread A again
3. Thread A continue ajoy_close and release opriv
4. Thread B resume and touch the freed memory
Basically, the resource release is very very special, you can't protect the resource by the normal approach. That's why I remove the related code from these patches, because it can't work, and just make the confusion.
--
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] [incubator-nuttx] pkarashchenko commented on pull request #5204: input/ajoystck: Always protect the au_open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#issuecomment-1020064786
I found a similar issue in `drivers/input/button_upper.c`
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815466236
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
IMO the semaphore protects `priv` and critical section protects `opriv` + iteration thru the linked list
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815454108
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
I agree with buttons and djoystick changes, but first look here leads me to the next findings in `boards/arm/stm32/nucleo-f4x1re/src/stm32_ajoystick.c`:
```
static int ajoy_sample(FAR const struct ajoy_lowerhalf_s *lower,
FAR struct ajoy_sample_s *sample)
{
#ifndef NO_JOYSTICK_ADC
struct adc_msg_s adcmsg[MAX_ADC_CHANNELS];
FAR struct adc_msg_s *ptr;
ssize_t nread;
ssize_t offset;
int have;
int i;
/* Read all of the available samples (handling the case where additional
* channels are enabled).
*/
nread = file_read(&g_adcfile, adcmsg,
MAX_ADC_CHANNELS * sizeof(struct adc_msg_s));
if (nread < 0)
{
if (nread != -EINTR)
{
ierr("ERROR: read failed: %d\n", (int)nread);
}
return nread;
}
...
```
So the question is: Is it safe to call `file_read` from the critical section?
--
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] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r816948394
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
@patacongo I will appreciate your input 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