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@code...

---

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);

Hi Matt,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cb560795c8c2ceca1d36a95f0d1b2eafc4074e37]

url: https://github.com/intel-lab-lkp/linux/commits/Matt-Johnston/mctp-i2c-handle... base: cb560795c8c2ceca1d36a95f0d1b2eafc4074e37 patch link: https://lore.kernel.org/r/20241021-mctp-i2c-null-dest-v2-1-4503e478517c%40co... patch subject: [PATCH net v2] mctp i2c: handle NULL header address config: alpha-randconfig-r122-20241022 (https://download.01.org/0day-ci/archive/20241022/202410221734.IWc5paM1-lkp@i...) compiler: alpha-linux-gcc (GCC) 13.3.0 reproduce: (https://download.01.org/0day-ci/archive/20241022/202410221734.IWc5paM1-lkp@i...)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202410221734.IWc5paM1-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)

...

drivers/net/mctp/mctp-i2c.c:599:23: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned char [assigned] [usertype] llsrc @@ got unsigned char const *dev_addr @@

drivers/net/mctp/mctp-i2c.c:599:23: sparse: expected unsigned char [assigned] [usertype] llsrc drivers/net/mctp/mctp-i2c.c:599:23: sparse: got unsigned char const *dev_addr drivers/net/mctp/mctp-i2c.c: note: in included file (through include/linux/module.h): include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +599 drivers/net/mctp/mctp-i2c.c

579 580 static int mctp_i2c_header_create(struct sk_buff *skb, struct net_device *dev, 581 unsigned short type, const void *daddr, 582 const void *saddr, unsigned int len) 583 { 584 struct mctp_i2c_hdr *hdr; 585 struct mctp_hdr *mhdr; 586 u8 lldst, llsrc; 587 588 if (len > MCTP_I2C_MAXMTU) 589 return -EMSGSIZE; 590 591 if (daddr) 592 lldst = *((u8 *)daddr); 593 else 594 return -EINVAL; 595 596 if (saddr) 597 llsrc = *((u8 *)saddr); 598 else

599 llsrc = dev->dev_addr;

600 601 skb_push(skb, sizeof(struct mctp_i2c_hdr)); 602 skb_reset_mac_header(skb); 603 hdr = (void *)skb_mac_header(skb); 604 mhdr = mctp_hdr(skb); 605 hdr->dest_slave = (lldst << 1) & 0xff; 606 hdr->command = MCTP_I2C_COMMANDCODE; 607 hdr->byte_count = len + 1; 608 hdr->source_slave = ((llsrc << 1) & 0xff) | 0x01; 609 mhdr->ver = 0x01; 610 611 return sizeof(struct mctp_i2c_hdr); 612 } 613