[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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(>au_exclsem);` or inside the `ajoy_takesem(>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(>au_exclsem);` or inside the `ret = ajoy_takesem(>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(>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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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(>au_exclsem);` or inside the `ajoy_takesem(>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(>au_exclsem);` or inside the `ret = ajoy_takesem(>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(>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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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(>au_exclsem);` or inside the `ajoy_takesem(>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(>au_exclsem);` or inside the `ret = ajoy_takesem(>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(>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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@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
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(>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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org