[1/1] greybus: gb-beagleplay: Remove use of pad bytes

  Make gb-beagleplay greybus spec compliant by using a wrapper around HDLC
payload to include cport information.

Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver")
Reported-by: kernel test robot <yujie.liu@intel.com>
Closes: https://lore.kernel.org/r/202311072329.Xogj7hGW-lkp@intel.com/
Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
---
 drivers/greybus/gb-beagleplay.c | 44 +++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 13 deletions(-)
  

On Wed, Dec 06, 2023 at 08:36:00PM +0530, Ayush Singh wrote:
> Make gb-beagleplay greybus spec compliant by using a wrapper around HDLC
> payload to include cport information.

"wrapper"?  You just changed the data you send on your "wire", right?

> Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver")
> Reported-by: kernel test robot <yujie.liu@intel.com>
> Closes: https://lore.kernel.org/r/202311072329.Xogj7hGW-lkp@intel.com/
> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
> ---
>  drivers/greybus/gb-beagleplay.c | 44 +++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> index 1e70ff7e3da4..fb40ade9364f 100644
> --- a/drivers/greybus/gb-beagleplay.c
> +++ b/drivers/greybus/gb-beagleplay.c
> @@ -85,17 +85,35 @@ struct hdlc_payload {
>  	void *buf;
>  };
>  
> +/**
> + * struct hdlc_greybus_frame - Structure to represent greybus HDLC frame

Represent it where?  And where is this documented?

> + *
> + * @cport: cport id
> + * @hdr: greybus operation header
> + * @payload: greybus message payload
> + */
> +struct hdlc_greybus_frame {
> +	__le16 cport;
> +	struct gb_operation_msg_hdr hdr;
> +	u8 payload[];
> +} __packed;
> +
>  static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>  {
> -	u16 cport_id;
> -	struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf;
> +	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
> +	u16 cport_id = le16_to_cpu(gb_frame->cport);
>  
> -	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
> +	/* Ensure that the greybus message is valid */
> +	if (le16_to_cpu(gb_frame->hdr.size) > len - sizeof(cport_id)) {
> +		dev_warn_ratelimited(&bg->sd->dev, "Invalid/Incomplete greybus message");

Don't spam the kernel log for corrupted data on the line, that would be
a mess.  Use a tracepoint?

> +		return;
> +	}
>  
>  	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
> -		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
> +		gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result);

Better yet, put the error in the debug message?

>  
> -	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
> +	greybus_data_rcvd(bg->gb_hd, cport_id, &buf[sizeof(cport_id)],

Fun with pointer math.  This feels really fragile, why not just point to
the field instead?

> +			  le16_to_cpu(gb_frame->hdr.size));

You convert the size twice, might as well do it once up above, right?

Also, it's best to use the same value you checked as the value you pass
in here, odds are you can't change the data underneath you (like you
could if this came from userspace), but it's best to not get in the
habit of coding like this.


>  }
>  
>  static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
> @@ -339,7 +357,7 @@ static struct serdev_device_ops gb_beagleplay_ops = {
>  static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask)
>  {
>  	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
> -	struct hdlc_payload payloads[2];
> +	struct hdlc_payload payloads[3];

why 3?

It's ok to put this on the stack?

>  	__le16 cport_id = le16_to_cpu(cport);
>  
>  	dev_dbg(&hd->dev, "Sending greybus message with Operation %u, Type: %X on Cport %u",
> @@ -348,14 +366,14 @@ static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_messa
>  	if (le16_to_cpu(msg->header->size) > RX_HDLC_PAYLOAD)
>  		return dev_err_probe(&hd->dev, -E2BIG, "Greybus message too big");
>  
> -	memcpy(msg->header->pad, &cport_id, sizeof(cport_id));
> -
> -	payloads[0].buf = msg->header;
> -	payloads[0].len = sizeof(*msg->header);
> -	payloads[1].buf = msg->payload;
> -	payloads[1].len = msg->payload_size;
> +	payloads[0].buf = &cport_id;
> +	payloads[0].len = sizeof(cport_id);
> +	payloads[1].buf = msg->header;
> +	payloads[1].len = sizeof(*msg->header);
> +	payloads[2].buf = msg->payload;
> +	payloads[2].len = msg->payload_size;

So you send the cport number as the first message?  Don't you need to
document this somewhere?

thanks,

greg k-h
  

>> + *
>> + * @cport: cport id
>> + * @hdr: greybus operation header
>> + * @payload: greybus message payload
>> + */
>> +struct hdlc_greybus_frame {
>> +	__le16 cport;
>> +	struct gb_operation_msg_hdr hdr;
>> +	u8 payload[];
>> +} __packed;
>> +
>>   static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>>   {
>> -	u16 cport_id;
>> -	struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf;
>> +	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
>> +	u16 cport_id = le16_to_cpu(gb_frame->cport);
>>   
>> -	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
>> +	/* Ensure that the greybus message is valid */
>> +	if (le16_to_cpu(gb_frame->hdr.size) > len - sizeof(cport_id)) {
>> +		dev_warn_ratelimited(&bg->sd->dev, "Invalid/Incomplete greybus message");
> Don't spam the kernel log for corrupted data on the line, that would be
> a mess.  Use a tracepoint?
>
>> +		return;
>> +	}
>>   
>>   	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
>> -		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
>> +		gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result);
> Better yet, put the error in the debug message?
Shouldn't corrupt data be a warning rather than debug message, since it 
indicates something wrong with the transport?
>>   
>> -	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
>> +	greybus_data_rcvd(bg->gb_hd, cport_id, &buf[sizeof(cport_id)],
> Fun with pointer math.  This feels really fragile, why not just point to
> the field instead?
It seems that taking address of members of packed structures is not 
valid. I get the `address-of-packed-member` warnings. Is it fine to 
ignore those in kernel?
>>   }
>>   
>>   static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
>> @@ -339,7 +357,7 @@ static struct serdev_device_ops gb_beagleplay_ops = {
>>   static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask)
>>   {
>>   	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
>> -	struct hdlc_payload payloads[2];
>> +	struct hdlc_payload payloads[3];
> why 3?
>
> It's ok to put this on the stack?

Well, the HDLC payload is just to store the length of the payload along 
with a pointer to its data. (kind of emulate a fat pointer). The reason 
for doing it this way is to avoid having to create a temp buffer for 
each message when sending data over UART (which was done in the initial 
version of the driver).

Ayush Singh
  

On Thu, Dec 07, 2023 at 10:33:54PM +0530, Ayush Singh wrote:
> > > + *
> > > + * @cport: cport id
> > > + * @hdr: greybus operation header
> > > + * @payload: greybus message payload
> > > + */
> > > +struct hdlc_greybus_frame {
> > > +	__le16 cport;
> > > +	struct gb_operation_msg_hdr hdr;
> > > +	u8 payload[];
> > > +} __packed;
> > > +
> > >   static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
> > >   {
> > > -	u16 cport_id;
> > > -	struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf;
> > > +	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
> > > +	u16 cport_id = le16_to_cpu(gb_frame->cport);
> > > -	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
> > > +	/* Ensure that the greybus message is valid */
> > > +	if (le16_to_cpu(gb_frame->hdr.size) > len - sizeof(cport_id)) {
> > > +		dev_warn_ratelimited(&bg->sd->dev, "Invalid/Incomplete greybus message");
> > Don't spam the kernel log for corrupted data on the line, that would be
> > a mess.  Use a tracepoint?
> > 
> > > +		return;
> > > +	}
> > >   	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
> > > -		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
> > > +		gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result);
> > Better yet, put the error in the debug message?
> Shouldn't corrupt data be a warning rather than debug message, since it
> indicates something wrong with the transport?

Do you want messages like that spamming the kernel log all the time if a
network connection is corrupted?

Just handle the error and let the upper layers deal with it when the
problem is eventually reported to userspace, that's all that is needed.


> > > -	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
> > > +	greybus_data_rcvd(bg->gb_hd, cport_id, &buf[sizeof(cport_id)],
> > Fun with pointer math.  This feels really fragile, why not just point to
> > the field instead?
> It seems that taking address of members of packed structures is not valid.

That feels really odd.

> I get the `address-of-packed-member` warnings. Is it fine to ignore
> those in kernel?

What error exactly are you getting?  Packed or not does not mean
anything to the address of a member.  If it does, perhaps you are doing
something wrong as you are really doing the same thing here, right?
Don't ignore the warning by open-coding it.

> > >   }
> > >   static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
> > > @@ -339,7 +357,7 @@ static struct serdev_device_ops gb_beagleplay_ops = {
> > >   static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask)
> > >   {
> > >   	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
> > > -	struct hdlc_payload payloads[2];
> > > +	struct hdlc_payload payloads[3];
> > why 3?
> > 
> > It's ok to put this on the stack?
> 
> Well, the HDLC payload is just to store the length of the payload along with
> a pointer to its data. (kind of emulate a fat pointer). The reason for doing
> it this way is to avoid having to create a temp buffer for each message when
> sending data over UART (which was done in the initial version of the
> driver).

Be careful, are you SURE you are allowed to send stack-allocated data?
I know that many busses forbid this (like USB).  So please check to be
sure that this is ok to do, and that these are not huge structures that
you are putting on the very-limited kernel-stack.

thanks,

greg k-h
  

On 12/8/23 11:03, Greg KH wrote:

> On Thu, Dec 07, 2023 at 10:33:54PM +0530, Ayush Singh wrote:
>>>> + *
>>>> + * @cport: cport id
>>>> + * @hdr: greybus operation header
>>>> + * @payload: greybus message payload
>>>> + */
>>>> +struct hdlc_greybus_frame {
>>>> +	__le16 cport;
>>>> +	struct gb_operation_msg_hdr hdr;
>>>> +	u8 payload[];
>>>> +} __packed;
>>>> +
>>>>    static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>>>>    {
>>>> -	u16 cport_id;
>>>> -	struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf;
>>>> +	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
>>>> +	u16 cport_id = le16_to_cpu(gb_frame->cport);
>>>> -	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
>>>> +	/* Ensure that the greybus message is valid */
>>>> +	if (le16_to_cpu(gb_frame->hdr.size) > len - sizeof(cport_id)) {
>>>> +		dev_warn_ratelimited(&bg->sd->dev, "Invalid/Incomplete greybus message");
>>> Don't spam the kernel log for corrupted data on the line, that would be
>>> a mess.  Use a tracepoint?
>>>
>>>> +		return;
>>>> +	}
>>>>    	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
>>>> -		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
>>>> +		gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result);
>>> Better yet, put the error in the debug message?
>> Shouldn't corrupt data be a warning rather than debug message, since it
>> indicates something wrong with the transport?
> Do you want messages like that spamming the kernel log all the time if a
> network connection is corrupted?
>
> Just handle the error and let the upper layers deal with it when the
> problem is eventually reported to userspace, that's all that is needed.

Ok

>>>> -	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
>>>> +	greybus_data_rcvd(bg->gb_hd, cport_id, &buf[sizeof(cport_id)],
>>> Fun with pointer math.  This feels really fragile, why not just point to
>>> the field instead?
>> It seems that taking address of members of packed structures is not valid.
> That feels really odd.
>
>> I get the `address-of-packed-member` warnings. Is it fine to ignore
>> those in kernel?
> What error exactly are you getting?  Packed or not does not mean
> anything to the address of a member.  If it does, perhaps you are doing
> something wrong as you are really doing the same thing here, right?
> Don't ignore the warning by open-coding it.

So, the error I was getting was `taking address of packed member of 
'gb_frame' may result in an unaligned pointer value`. I can no longer 
reproduce the warning, though. I think I accidentally fixed the reason 
somewhere along the line.

>>>>    }
>>>>    static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
>>>> @@ -339,7 +357,7 @@ static struct serdev_device_ops gb_beagleplay_ops = {
>>>>    static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask)
>>>>    {
>>>>    	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
>>>> -	struct hdlc_payload payloads[2];
>>>> +	struct hdlc_payload payloads[3];
>>> why 3?
>>>
>>> It's ok to put this on the stack?
>> Well, the HDLC payload is just to store the length of the payload along with
>> a pointer to its data. (kind of emulate a fat pointer). The reason for doing
>> it this way is to avoid having to create a temp buffer for each message when
>> sending data over UART (which was done in the initial version of the
>> driver).
> Be careful, are you SURE you are allowed to send stack-allocated data?
> I know that many busses forbid this (like USB).  So please check to be
> sure that this is ok to do, and that these are not huge structures that
> you are putting on the very-limited kernel-stack.

Well, the greybus operation header and greybus message are not on the 
stack (as far as I know, since they are inputs to the function). The 
memory that is stack allocated for hdlc_payload array is `3 * 
(sizeof(u16) + sizeof(void *))`, i.e. 30 bytes for 64 bit pointers 
(ignoring any padding). So I don't think the size of hdlc_payload array 
should be an issue.

The function `hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 3);` 
actually copies the data to a different circ_buf, which is sent later by 
workqueue. So the final data sent is not stack allocated. In fact, none 
of the data from this function is going to be referenced after the call 
to `hdlc_tx_frames`.


Ayush Singh