Messages in this thread | | | Date | Fri, 3 Nov 2023 19:07:03 -0700 | From | Richard Cochran <> | Subject | Re: [PATCH net-next V3] ptp: fix corrupted list in ptp_open |
| |
On Fri, Nov 03, 2023 at 09:15:03PM +0800, Edward Adam Davis wrote: > There is no lock protection when writing ptp->tsevqs in ptp_open(), > ptp_release(), which can cause data corruption, use mutex lock to avoid this > issue. > > Moreover, ptp_release() should not be used to release the queue in ptp_read(), > and it should be deleted together.
Oh, now I see what you are fixing...
> @@ -138,14 +143,19 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode) > int ptp_release(struct posix_clock_context *pccontext) > { > struct timestamp_event_queue *queue = pccontext->private_clkdata; > + struct ptp_clock *ptp = > + container_of(pccontext->clk, struct ptp_clock, clock); > unsigned long flags; > > if (queue) { > + if (mutex_lock_interruptible(&ptp->tsevq_mux)) > + return -ERESTARTSYS;
I don't think it is a good idea to return ERESTARTSYS on signal here. The release method needs to succeed.
> debugfs_remove(queue->debugfs_instance); > pccontext->private_clkdata = NULL; > spin_lock_irqsave(&queue->lock, flags);
This spin lock is wrong. The spin lock protects the queue, not the list of queues.
The spin lock/unlock needs to be replaced with mutex lock/unlock.
> list_del(&queue->qlist); > spin_unlock_irqrestore(&queue->lock, flags); > + mutex_unlock(&ptp->tsevq_mux); > bitmap_free(queue->mask); > kfree(queue); > }
> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > index 52f87e394aa6..1525bd2059ba 100644 > --- a/drivers/ptp/ptp_private.h > +++ b/drivers/ptp/ptp_private.h > @@ -44,6 +44,7 @@ struct ptp_clock { > struct pps_device *pps_source; > long dialed_frequency; /* remembers the frequency adjustment */ > struct list_head tsevqs; /* timestamp fifo list */ > + struct mutex tsevq_mux; /* one process at a time reading the fifo */
This comment is very misleading. The mutex does not protect the fifo. It protects 'tsevqs' from concurrent access.
Thanks, Richard
| |