Re: [PATCH net-next v4 4/6] ptp: support event queue reader channel masks

[Simon Horman <horms@kernel.org>]

On Thu, Oct 05, 2023 at 03:53:14PM +0200, Xabier Marquiegui wrote:
> On systems with multiple timestamp event channels, some readers might
> want to receive only a subset of those channels.
> 
> This patch adds the necessary modifications to support timestamp event
> channel filtering, including two IOCTL operations:
> 
> - Clear all channels
> - Enable one channel
> 
> The mask modification operations will be applied exclusively on the
> event queue assigned to the file descriptor used on the IOCTL operation,
> so the typical procedure to have a reader receiving only a subset of the
> enabled channels would be:
> 
> - Open device file
> - ioctl: clear all channels
> - ioctl: enable one channel
> - start reading
> 
> Calling the enable one channel ioctl more than once will result in
> multiple enabled channels.
> 
> Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
> Suggested-by: Richard Cochran <richardcochran@gmail.com>
> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> v4:
>   - split modifications in different patches for improved organization
>   - filter modifications exclusive to currently open instance for
>     simplicity and security
>   - expand mask to 2048 channels
>   - remove unnecessary tests
> v3: https://lore.kernel.org/netdev/20230928133544.3642650-4-reibax@gmail.com/
>   - filter application by object id, aided by process id
>   - friendlier testptp implementation of event queue channel filters
> v2: https://lore.kernel.org/netdev/20230912220217.2008895-3-reibax@gmail.com/
>   - fix testptp compilation error: unknown type name 'pid_t'
>   - rename mask variable for easier code traceability
>   - more detailed commit message with two examples
> v1: https://lore.kernel.org/netdev/20230906104754.1324412-4-reibax@gmail.com/
> ---
>  drivers/ptp/ptp_chardev.c      | 24 ++++++++++++++++++++++++
>  drivers/ptp/ptp_clock.c        | 12 ++++++++++--
>  drivers/ptp/ptp_private.h      |  3 +++
>  include/uapi/linux/ptp_clock.h |  2 ++
>  4 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index abe94bb80cf6..dbbe551a044f 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -110,6 +110,10 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
>  	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
>  	if (!queue)
>  		return -EINVAL;
> +	queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL);
> +	if (!queue->mask)

Hi Xabier,

queue appears to be leaked here.

As flagged by Smatch.

> +		return -EINVAL;
> +	bitmap_set(queue->mask, 0, PTP_MAX_CHANNELS);
>  	spin_lock_init(&queue->lock);
>  	list_add_tail(&queue->qlist, &ptp->tsevqs);
>  	pccontext->private_clkdata = queue;