From: Johan Hovold <johan@kernel.org>
To: Octavian Purdila <octavian.purdila@intel.com>
Cc: Johan Hovold <johan@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Linus Walleij <linus.walleij@linaro.org>,
Alexandre Courbot <gnurou@gmail.com>,
wsa@the-dreams.de, Samuel Ortiz <sameo@linux.intel.com>,
Lee Jones <lee.jones@linaro.org>, Arnd Bergmann <arnd@arndb.de>,
Daniel Baluta <daniel.baluta@intel.com>,
Laurentiu Palcu <laurentiu.palcu@intel.com>,
linux-usb@vger.kernel.org, lkml <linux-kernel@vger.kernel.org>,
linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org
Subject: Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter
Date: Thu, 18 Sep 2014 11:13:13 +0200 [thread overview]
Message-ID: <20140918091313.GA1989@localhost> (raw)
In-Reply-To: <CAE1zotKB48T9qz4NaxMbMMiuit_v+Zd9wpcdQrVFw7nVCBYX-w@mail.gmail.com>
On Thu, Sep 18, 2014 at 11:49:19AM +0300, Octavian Purdila wrote:
> On Thu, Sep 18, 2014 at 11:19 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Sep 17, 2014 at 01:07:51PM +0300, Octavian Purdila wrote:
> >> On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@kernel.org> wrote:
> >>
> >> <snip>
> >>
> >> >> + /*
> >> >> + * Buffer to hold the packet for read or write transfers. One
> >> >> + * is enough since we can't have multiple transfers in
> >> >> + * parallel on the i2c adapter.
> >> >> + */
> >> >> + union {
> >> >> + struct {
> >> >> + u8 port;
> >> >> + u8 addr;
> >> >> + u8 mem_addr_len;
> >> >> + __le32 mem_addr;
> >> >> + __le16 buf_len;
> >> >> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> >> + } __packed tx;
> >> >> + struct {
> >> >> + __le16 buf_len;
> >> >> + u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> >> + } __packed rx;
> >> >> + } buf;
> >> >
> >> > While this works in this case due to the extra copy you do in
> >> > dln2_transfer, allocating buffers that would (generally) be used for DMA
> >> > transfers as part of a larger structure is a recipe for trouble.
> >> >
> >> > It's probably better to allocate separately, if only to prevent people
> >> > from thinking there might be a bug here.
> >> >
> >>
> >> Just to make sure I understand this, what could the issues be? The
> >> buffers not being aligned or not allocated in continuous physical
> >> memory?
> >
> > Yes, the buffer (and any subsequent field) would have to be cache-line
> > aligned to avoid corruption due to cache-line sharing on some systems.
> >
>
> Ah, ok, makes sense now. But is it safe to use kmalloc() in this case?
> Does kmalloc() prevent cache line sharing?
Yes, it does (as long as you allocate the buffer separately from the
containing struct).
> >> <snip>
> >>
> >> >> +
> >> >> + rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
> >> >> + if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
> >> >> + return -EPROTO;
> >> >> +
> >> >> + if (data_len > rx_buf_len)
> >> >> + data_len = rx_buf_len;
> >> >
> >> > You're still not checking that the received data does not overflow the
> >> > supplied buffer as I already commented on v3.
> >> >
> >> >> +
> >> >> + memcpy(data, dln2->buf.rx.buf, data_len);
> >> >> +
> >> >> + return data_len;
> >> >> +}
> >>
> >> Hmm, perhaps I am missing something, but we never transfer more then
> >> data_len, where data_len is the size of the buffer supplied by the
> >> user.
> >
> > That is the amount of data you request from the device, but you never
> > check how much is actually returned.
> >
>
> Actually we check the receive buffer size here:
>
> if (data_len > rx_buf_len)
> data_len = rx_buf_len;
>
> rx_buf_len is the i2c received payload size while rx_len is the length
> of received message
Bah, you're right. You never explicitly check for overflow, but also
never use more than data_len bytes of the returned buffer.
I think you should add explicit checks, and return an error in this case
rather than silently truncate the data.
> > You really should clean up the error handling of this function as it is
> > currently not very readable.
> >
>
> Perhaps adding some comments similar to the the explanation above would help?
It's more the logic of this function I find a bit twisted. I think you
should clean it up and consider adding appropriately named (temporary)
variables to make it more readable.
Johan
next prev parent reply other threads:[~2014-09-18 9:15 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-09 19:24 [PATCH v4 0/3] mfd: add support for Diolan DLN-2 Octavian Purdila
2014-09-09 19:24 ` [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices Octavian Purdila
2014-09-16 23:21 ` Lee Jones
2014-09-17 7:25 ` Octavian Purdila
2014-09-17 7:38 ` Johan Hovold
2014-09-17 15:56 ` Lee Jones
2014-09-17 9:10 ` Johan Hovold
2014-09-17 15:46 ` Lee Jones
2014-09-18 11:31 ` Johan Hovold
2014-09-18 14:21 ` Johan Hovold
2014-09-09 19:24 ` [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter Octavian Purdila
2014-09-17 9:44 ` Johan Hovold
2014-09-17 10:07 ` Octavian Purdila
2014-09-18 8:19 ` Johan Hovold
2014-09-18 8:49 ` Octavian Purdila
2014-09-18 9:13 ` Johan Hovold [this message]
2014-09-09 19:24 ` [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver Octavian Purdila
2014-09-18 10:54 ` Johan Hovold
2014-09-18 12:43 ` Octavian Purdila
2014-09-18 12:46 ` Johan Hovold
2014-09-18 15:54 ` Octavian Purdila
2014-09-19 7:11 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140918091313.GA1989@localhost \
--to=johan@kernel.org \
--cc=arnd@arndb.de \
--cc=daniel.baluta@intel.com \
--cc=gnurou@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=laurentiu.palcu@intel.com \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=octavian.purdila@intel.com \
--cc=sameo@linux.intel.com \
--cc=wsa@the-dreams.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).