Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

[Simon Horman <horms@kernel.org>]

On Thu, Sep 28, 2023 at 03:35:44PM +0200, Xabier Marquiegui wrote:
> Implement ioctl to support filtering of external timestamp event queue
> channels per reader based on the process PID accessing the timestamp
> queue.
> 
> Can be tested using testptp test binary. Use lsof to figure out readers
> of the DUT. LSB of the timestamp channel mask is channel 0.
> 
> eg: To view all current users of the device:
> ```
>  # testptp -F  /dev/ptp0 
> (USER PID)     TSEVQ FILTER ID:MASK
> (3234)              1:0x00000001
> (3692)              2:0xFFFFFFFF
> (3792)              3:0xFFFFFFFF
> (8713)              4:0xFFFFFFFF
> ```
> 
> eg: To allow ID 1 to access only ts channel 0:
> ```
>  # testptp -F 1,0x1
> ```
> 
> eg: To allow ID 1 to access any channel:
> ```
>  # testptp -F 1,0xFFFFFFFF
> ```
> 
> Signed-off-by: Xabier Marquiegui <reibax@gmail.com>
> Suggested-by: Richard Cochran <richardcochran@gmail.com>

Hi Xabier,

please find some more feedback from Smatch inline.

...

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c

...

> @@ -169,19 +170,28 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd,
>  {
>  	struct ptp_clock *ptp =
>  		container_of(pcuser->clk, struct ptp_clock, clock);
> +	struct ptp_tsfilter tsfilter_set, *tsfilter_get = NULL;
>  	struct ptp_sys_offset_extended *extoff = NULL;
>  	struct ptp_sys_offset_precise precise_offset;
>  	struct system_device_crosststamp xtstamp;
>  	struct ptp_clock_info *ops = ptp->info;
>  	struct ptp_sys_offset *sysoff = NULL;
> +	struct timestamp_event_queue *tsevq;
>  	struct ptp_system_timestamp sts;
>  	struct ptp_clock_request req;
>  	struct ptp_clock_caps caps;
>  	struct ptp_clock_time *pct;
> +	int lsize, enable, err = 0;
>  	unsigned int i, pin_index;
>  	struct ptp_pin_desc pd;
>  	struct timespec64 ts;
> -	int enable, err = 0;
> +
> +	tsevq = pcuser->private_clkdata;
> +
> +	if (tsevq->close_req) {
> +		err = -EPIPE;
> +		return err;
> +	}

Here tseqv is dereferenced unconditionally...

>  
>  	switch (cmd) {
>  
> @@ -481,6 +491,79 @@ long ptp_ioctl(struct posix_clock_user *pcuser, unsigned int cmd,
>  		mutex_unlock(&ptp->pincfg_mux);
>  		break;
>  
> +	case PTP_FILTERCOUNT_REQUEST:
> +		/* Calculate amount of device users */
> +		if (tsevq) {

... but here it is assumed that tseqv might be NULL.

As flagged by Smatch.

> +			lsize = list_count_nodes(&tsevq->qlist);
> +			if (copy_to_user((void __user *)arg, &lsize,
> +					 sizeof(lsize)))
> +				err = -EFAULT;
> +		}
> +		break;
> +	case PTP_FILTERTS_GET_REQUEST:
> +		/* Read operation */
> +		/* Read amount of entries expected */
> +		if (copy_from_user(&tsfilter_set, (void __user *)arg,
> +				   sizeof(tsfilter_set))) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		if (tsfilter_set.ndevusers <= 0) {
> +			err = -EINVAL;
> +			break;
> +		}
> +		/* Allocate the necessary memory space to dump the requested filter
> +		 * list
> +		 */
> +		tsfilter_get = kzalloc(tsfilter_set.ndevusers *
> +					       sizeof(struct ptp_tsfilter),
> +				       GFP_KERNEL);
> +		if (!tsfilter_get) {
> +			err = -ENOMEM;
> +			break;
> +		}
> +		if (!tsevq) {

Ditto.

> +			err = -EFAULT;
> +			break;
> +		}
> +		/* Set the whole region to 0 in case the current list is shorter than
> +		 * anticipated
> +		 */
> +		memset(tsfilter_get, 0,
> +		       tsfilter_set.ndevusers * sizeof(struct ptp_tsfilter));
> +		i = 0;
> +		/* Format data */
> +		list_for_each_entry(tsevq, &ptp->tsevqs, qlist) {
> +			tsfilter_get[i].reader_rpid = tsevq->reader_pid;
> +			tsfilter_get[i].reader_oid = tsevq->oid;
> +			tsfilter_get[i].mask = tsevq->mask;
> +			i++;
> +			/* Current list is longer than anticipated */
> +			if (i >= tsfilter_set.ndevusers)
> +				break;
> +		}
> +		/* Dump data */
> +		if (copy_to_user((void __user *)arg, tsfilter_get,
> +				 tsfilter_set.ndevusers *
> +					 sizeof(struct ptp_tsfilter)))
> +			err = -EFAULT;
> +		break;
> +
> +	case PTP_FILTERTS_SET_REQUEST:
> +		/* Write Operation */
> +		if (copy_from_user(&tsfilter_set, (void __user *)arg,
> +				   sizeof(tsfilter_set))) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		if (tsevq) {

Ditto.

> +			list_for_each_entry(tsevq, &ptp->tsevqs, qlist) {
> +				if (tsevq->oid == tsfilter_set.reader_oid)
> +					tsevq->mask = tsfilter_set.mask;
> +			}
> +		}
> +		break;
> +
>  	default:
>  		err = -ENOTTY;
>  		break;

...