Re: [PATCH mptcp-next 3/5] mptcp: add MPTCP_TCPINFO getsockopt support

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Wed, 11 Aug 2021, Florian Westphal wrote:

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index eba294a071c8..ac8e6823db4f 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -723,6 +723,96 @@ static int mptcp_getsockopt_info(struct mptcp_sock *msk, char __user *optval, in
>
> 	if (copy_to_user(optval, &m_info, len))
> 		return -EFAULT;
> +	return 0;
> +}
> +
> +static int mptcp_get_subflow_data(struct mptcp_subflow_data *sfd,
> +				  char __user *optval, int __user *_u_optlen)
> +{
> +#define MIN_INFO_OPTLEN_SIZE	16

This caught my eye on my first quick review pass, I'm not accustomed to 
seeing inline defines like this in the kernel unless it's part of some 
macro magic. The style guide / checkpatch don't say anything, so I'll just 
suggest it would fit better outside the function scope (but no big deal).


For the rest of the patch set, Paolo already covered the expanded test 
cases, so I'll continue with a more in-depth review when you post v2. 
This iteration of the API is looking like the right direction to me.


Thanks,

Mat


> +	int len, copylen;
> +
> +	if (get_user(len, _u_optlen))
> +		return -EFAULT;
> +
> +	if (len <= MIN_INFO_OPTLEN_SIZE)
> +		return -EINVAL;
> +
> +	memset(sfd, 0, sizeof(*sfd));
> +
> +	copylen = min_t(unsigned int, len, sizeof(*sfd));
> +	if (copy_from_user(sfd, optval, copylen))
> +		return -EFAULT;
> +
> +	/* size_subflow_data is u32, but len is signed */
> +	if (sfd->size_subflow_data > INT_MAX ||
> +	    sfd->size_user > INT_MAX)
> +		return -EINVAL;
> +
> +	if (sfd->size_subflow_data < MIN_INFO_OPTLEN_SIZE ||
> +	    sfd->size_subflow_data > len)
> +		return -EINVAL;
> +
> +	if (sfd->num_subflows || sfd->size_kernel)
> +		return -EINVAL;
> +
> +	return len - sfd->size_subflow_data;
> +}

--
Mat Martineau
Intel