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@xxxxxxxxxxxxxxx
> Reported-by: Dung Cao <dung@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Matt Johnston <matt@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxx
> ---
> 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
