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

[Ayush Singh]

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