Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix poll notify #14670

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+39 −49
Open

Fix poll notify #14670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03955cb
fs_poll: race condition protection when modifying revents value
t-salminen Nov 4, 2024
0ab693b
fs/vfs/fs_poll.c: Only enter critical section when actually setting r…
jlaitine Nov 6, 2024
bf584e9
Revert "serial.c: should protect the fds when calling poll_notify"
jlaitine Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 19 additions & 43 deletions drivers/serial/serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@
* Private Function Prototypes
****************************************************************************/

/* Poll support */

static void uart_poll_notify(FAR uart_dev_t *dev, unsigned int min,
unsigned int max, pollevent_t eventset);

/* Write support */

static int uart_putxmitchar(FAR uart_dev_t *dev, int ch,
Expand Down Expand Up @@ -202,28 +197,6 @@ static bool uart_is_termios_hw_change(FAR struct file *filep,
return false;
}

/****************************************************************************
* Name: uart_poll_notify
****************************************************************************/

static void uart_poll_notify(FAR uart_dev_t *dev, unsigned int min,
unsigned int max, pollevent_t eventset)
{
irqstate_t flags;

DEBUGASSERT(max > min && max - min <= CONFIG_SERIAL_NPOLLWAITERS);

flags = enter_critical_section();
sched_lock();

/* Notify the fds in range dev->fds[min] - dev->fds[max] */

poll_notify(&dev->fds[min], max - min, eventset);

sched_unlock();
leave_critical_section(flags);
}

/****************************************************************************
* Name: uart_putxmitchar
****************************************************************************/
Expand Down Expand Up @@ -832,6 +805,7 @@ static int uart_close(FAR struct file *filep)
nxmutex_destroy(&dev->xmit.lock);
nxmutex_destroy(&dev->recv.lock);
nxmutex_destroy(&dev->closelock);
nxmutex_destroy(&dev->polllock);
nxsem_destroy(&dev->xmitsem);
nxsem_destroy(&dev->recvsem);
uart_release(dev);
Expand Down Expand Up @@ -1751,9 +1725,8 @@ static int uart_poll(FAR struct file *filep,
FAR struct inode *inode = filep->f_inode;
FAR uart_dev_t *dev = inode->i_private;
pollevent_t eventset;
irqstate_t flags;
int ndx;
int ret = OK;
int ret;
int i;

/* Some sanity checking */
Expand All @@ -1765,10 +1738,18 @@ static int uart_poll(FAR struct file *filep,
}
#endif

flags = enter_critical_section();

/* Are we setting up the poll? Or tearing it down? */

ret = nxmutex_lock(&dev->polllock);
if (ret < 0)
{
/* A signal received while waiting for access to the poll data
* will abort the operation.
*/

return ret;
}

if (setup)
{
/* This is a request to set up the poll. Find an available
Expand Down Expand Up @@ -1796,8 +1777,6 @@ static int uart_poll(FAR struct file *filep,
goto errout;
}

leave_critical_section(flags);

/* Should we immediately notify on any of the requested events?
* First, check if the xmit buffer is full.
*
Expand Down Expand Up @@ -1846,7 +1825,7 @@ static int uart_poll(FAR struct file *filep,
}
#endif

uart_poll_notify(dev, i, i + 1, eventset);
poll_notify(&fds, 1, eventset);
}
else if (fds->priv != NULL)
{
Expand All @@ -1866,14 +1845,10 @@ static int uart_poll(FAR struct file *filep,

*slot = NULL;
fds->priv = NULL;

leave_critical_section(flags);
}

return ret;

errout:
leave_critical_section(flags);
nxmutex_unlock(&dev->polllock);
return ret;
}

Expand Down Expand Up @@ -1904,6 +1879,7 @@ static int uart_unlink(FAR struct inode *inode)
nxmutex_destroy(&dev->xmit.lock);
nxmutex_destroy(&dev->recv.lock);
nxmutex_destroy(&dev->closelock);
nxmutex_destroy(&dev->polllock);
nxsem_destroy(&dev->xmitsem);
nxsem_destroy(&dev->recvsem);
uart_release(dev);
Expand Down Expand Up @@ -2042,6 +2018,7 @@ int uart_register(FAR const char *path, FAR uart_dev_t *dev)
nxmutex_init(&dev->closelock);
nxsem_init(&dev->xmitsem, 0, 0);
nxsem_init(&dev->recvsem, 0, 0);
nxmutex_init(&dev->polllock);

#ifdef CONFIG_SERIAL_TERMIOS
dev->timeout = 0;
Expand Down Expand Up @@ -2077,7 +2054,7 @@ void uart_datareceived(FAR uart_dev_t *dev)
{
/* Notify all poll/select waiters that they can read from the recv buffer */

uart_poll_notify(dev, 0, CONFIG_SERIAL_NPOLLWAITERS, POLLIN);
poll_notify(dev->fds, CONFIG_SERIAL_NPOLLWAITERS, POLLIN);

/* Is there a thread waiting for read data? */

Expand Down Expand Up @@ -2111,7 +2088,7 @@ void uart_datasent(FAR uart_dev_t *dev)
{
/* Notify all poll/select waiters that they can write to xmit buffer */

uart_poll_notify(dev, 0, CONFIG_SERIAL_NPOLLWAITERS, POLLOUT);
poll_notify(dev->fds, CONFIG_SERIAL_NPOLLWAITERS, POLLOUT);

/* Is there a thread waiting for space in xmit.buffer? */

Expand Down Expand Up @@ -2150,7 +2127,6 @@ void uart_connected(FAR uart_dev_t *dev, bool connected)
*/

flags = enter_critical_section();
sched_lock();
dev->disconnected = !connected;
if (!connected)
{
Expand All @@ -2171,7 +2147,6 @@ void uart_connected(FAR uart_dev_t *dev, bool connected)
uart_wakeup(&dev->recvsem);
}

sched_unlock();
leave_critical_section(flags);
}
#endif
Expand All @@ -2192,6 +2167,7 @@ void uart_reset_sem(FAR uart_dev_t *dev)
nxsem_reset(&dev->recvsem, 0);
nxmutex_reset(&dev->xmit.lock);
nxmutex_reset(&dev->recv.lock);
nxmutex_reset(&dev->polllock);
}

/****************************************************************************
Expand Down
25 changes: 19 additions & 6 deletions fs/vfs/fs_poll.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,8 @@ void poll_notify(FAR struct pollfd **afds, int nfds, pollevent_t eventset)
{
int i;
FAR struct pollfd *fds;
pollevent_t revents;
irqstate_t flags;

DEBUGASSERT(afds != NULL && nfds >= 1);

Expand All @@ -286,16 +288,27 @@ void poll_notify(FAR struct pollfd **afds, int nfds, pollevent_t eventset)
{
/* The error event must be set in fds->revents */

fds->revents |= eventset & (fds->events | POLLERR | POLLHUP);
if ((fds->revents & (POLLERR | POLLHUP)) != 0)
revents = eventset & (fds->events | POLLERR | POLLHUP);
if (revents != 0)
{
/* Error or Hung up, clear POLLOUT event */
/* race condition protection when modifying fds->revents */

fds->revents &= ~POLLOUT;
flags = enter_critical_section();

fds->revents |= revents;
if ((fds->revents & (POLLERR | POLLHUP)) != 0)
{
/* Error or Hung up, clear POLLOUT event */

revents &= ~POLLOUT;
fds->revents &= ~POLLOUT;
}

leave_critical_section(flags);
}

if ((fds->revents != 0 || (fds->events & POLLALWAYS) != 0) &&
fds->cb != NULL)
if (fds->cb != NULL &&
(revents != 0 || (fds->events & POLLALWAYS)))
{
finfo("Report events: %08" PRIx32 "\n", fds->revents);
fds->cb(fds);
Expand Down
1 change: 1 addition & 0 deletions include/nuttx/serial/serial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ struct uart_dev_s
sem_t xmitsem; /* Wakeup user waiting for space in xmit.buffer */
sem_t recvsem; /* Wakeup user waiting for data in recv.buffer */
mutex_t closelock; /* Locks out new open while close is in progress */
mutex_t polllock; /* Manages exclusive access to fds[] */

/* I/O buffers */

Expand Down
Loading