Re: [PATCH net v2] mctp i2c: handle NULL header address

[Simon Horman <horms@kernel.org>]

On Mon, Oct 21, 2024 at 12:35:26PM +0800, Matt Johnston wrote:
> daddr can be NULL if there is no neighbour table entry present,
> in that case the tx packet should be dropped.
> 
> saddr will normally be set by MCTP core, but in case it is NULL it
> should be set to the device address.
> 
> Incorrect indent of the function arguments is also fixed.
> 
> Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
> Cc: stable@vger.kernel.org
> Reported-by: Dung Cao <dung@os.amperecomputing.com>
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
> Changes in v2:
> - Set saddr to device address if NULL, mention in commit message
> - Fix patch prefix formatting
> - Link to v1: https://lore.kernel.org/r/20241018-mctp-i2c-null-dest-v1-1-ba1ab52966e9@codeconstruct.com.au
> ---
>  drivers/net/mctp/mctp-i2c.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index 4dc057c121f5d0fb9c9c48bf16b6933ae2f7b2ac..c909254e03c21518c17daf8b813e610558e074c1 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c
> @@ -579,7 +579,7 @@ static void mctp_i2c_flow_release(struct mctp_i2c_dev *midev)
>  
>  static int mctp_i2c_header_create(struct sk_buff *skb, struct net_device *dev,
>  				  unsigned short type, const void *daddr,
> -	   const void *saddr, unsigned int len)
> +				  const void *saddr, unsigned int len)
>  {
>  	struct mctp_i2c_hdr *hdr;
>  	struct mctp_hdr *mhdr;

Hi Matt,

I think you should drop this hunk.
While it's nice to clean things up, in the context of other work [1],
this isn't really appropriate as part of a fix for net.

[1] https://docs.kernel.org/process/maintainer-netdev.html#clean-up-patches

> @@ -588,8 +588,15 @@ static int mctp_i2c_header_create(struct sk_buff *skb, struct net_device *dev,
>  	if (len > MCTP_I2C_MAXMTU)
>  		return -EMSGSIZE;
>  
> -	lldst = *((u8 *)daddr);
> -	llsrc = *((u8 *)saddr);
> +	if (daddr)
> +		lldst = *((u8 *)daddr);
> +	else
> +		return -EINVAL;
> +
> +	if (saddr)
> +		llsrc = *((u8 *)saddr);
> +	else
> +		llsrc = dev->dev_addr;

This last line doesn't seem right, as llsrc is a u8,
while dev->dev_addr is a pointer to unsigned char.

>  
>  	skb_push(skb, sizeof(struct mctp_i2c_hdr));
>  	skb_reset_mac_header(skb);

-- 
pw-bot: changes-requested