[OpenWrt-Devel] [PATCH v2] ubus: use network order in ubus message header fields

[Felix Fietkau]

On 2016-02-28 09:34, Eyal Birger wrote:
> Hi,
> 
> On Tue, Feb 16, 2016 at 12:21 PM Felix Fietkau <nbd at openwrt.org
> <mailto:nbd at openwrt.org>> wrote:
> 
>     On 2016-02-16 11:13, Eyal Birger wrote:
>     > Hi Felix, thanks for your patience.
>     >
>     > On Tue, Feb 16, 2016 at 12:00 PM Felix Fietkau <nbd at openwrt.org
>     <mailto:nbd at openwrt.org>
>     > <mailto:nbd at openwrt.org <mailto:nbd at openwrt.org>>> wrote:
>     >
>     >     On 2016-02-16 10:06, Eyal Birger wrote:
>     >     > Hi Felix,
>     >     >
>     >     > Thanks again for your responses.
>     >     >
>     >     > On Mon, Feb 15, 2016 at 2:14 PM Felix Fietkau
>     <nbd at openwrt.org <mailto:nbd at openwrt.org>
>     >     <mailto:nbd at openwrt.org <mailto:nbd at openwrt.org>>
>     >     > <mailto:nbd at openwrt.org <mailto:nbd at openwrt.org>
>     <mailto:nbd at openwrt.org <mailto:nbd at openwrt.org>>>> wrote:
>     >     >
>     >     >     On 2016-02-15 12:54, Eyal Birger wrote:
>     >     >     >     >     >       if (offset < sizeof(ub->hdr)) {
>     >     >     >     >     > -             iov[0].iov_base = ((char *)
>     >     &ub->hdr) +
>     >     >     offset;
>     >     >     >     >     > -             iov[0].iov_len =
>     sizeof(ub->hdr) -
>     >     offset;
>     >     >     >     >     > +             struct ubus_msghdr hdr;
>     >     >     >     >     > +
>     >     >     >     >     > +             hdr.version = ub->hdr.version;
>     >     >     >     >     > +             hdr.type = ub->hdr.type;
>     >     >     >     >     > +             hdr.seq =
>     cpu_to_be16(ub->hdr.seq);
>     >     >     >     >     > +             hdr.peer =
>     cpu_to_be32(ub->hdr.peer);
>     >     >     >     >     > +
>     >     >     >     >     > +             iov[0].iov_base = ((char *)
>     &hdr)
>     >     + offset;
>     >     >     >     >     > +             iov[0].iov_len = sizeof(hdr)
>     - offset;
>     >     >     >     The corner case is this: You changed the iov to
>     point at
>     >     stack
>     >     >     space
>     >     >     >     instead of ub->hdr. If the code receives a part of the
>     >     header
>     >     >     in one
>     >     >     >     call, and another one in the next (offset > 0),
>     the contents
>     >     >     of hdr will
>     >     >     >     be corrupt, as it will be a mix of uninitialized stack
>     >     space + the
>     >     >     >     received data from the last call.
>     >     >     > Interesting... I initialize the iov_base every time to
>     a newly
>     >     >     created and
>     >     >     > calculated hdr variable before the sendmsg() call, and
>     iov is
>     >     >     never used
>     >     >     > otherwise - so I wonder how it could be reused in
>     subsequent
>     >     calls?
>     >     >     Before your change, iov[0].iov_base points at ub->hdr, which
>     >     is on heap
>     >     >     and is preserved across calls.
>     >     >     After your change, iov[0].iov_base points at the on-stack
>     >     struct hdr,
>     >     >     which is not preserved across calls.
>     >     >
>     >     >
>     >     > The thing is, I don't see why the area pointed to in iov_base is
>     >     > required to be preserved between calls - the sendmsg() call
>     never uses
>     >     > the cached values in iov. They are always re-armed with new
>     pointers.
>     >
>     >     You're still confused, so please forget about iov for a second
>     and only
>     >     focus on struct ubus_msghdr hdr. Think about what happens if
>     the first
>     >     call only receives a few bytes of it, and the next call fills
>     in the
>     >     rest.
>     >
>     >
>     > Ok, I'm fine with the premise that I'm confused :)
>     Sorry, I meant to write 'you're still confusing me'.
> 
>     > so I'll try to explain my confusion:
>     >
>     > On the first call to ubus_msg_writev(offset = 0):
>     >   offset is less than sizeof(ub->hdr)
>     >     hdr is created on the stack, passed to sendmsg()
>     >     sendmsg() returns. No more references to hdr exist anywhere -
>     >       hdr contents is copied to the kernel socket for consumption.
>     >     but sendmsg() did not write all of the hdr (e.g. only 3 bytes).
>     >     ubus_msg_writev() returns with return value '3'.
>     >
>     > Second call to ubus_msg_writev(offset = 3):
>     >   offset is still less than sizeof(ub->hdr)
>     >     hdr is recreated on the stack, it's a different hdr now that
>     is sent
>     >         to sendmsg(), but contains the remaining bytes to be sent.
>     >
>     > That's the point I don't understand - the old pointer to hdr is
>     never used
>     > again. So the only way this would create problems is if somehow
>     the hdr
>     > pointer is retained after the sendmsg() call exits. But calling
>     > sendmsg() with iov's
>     > pointing to stack area is legitimate as far as I can tell.
>     It seems I got mixed up in reads vs writes and the fact that you
>     re-create the buffer every time. It looks a bit convoluted, but it seems
>     that it's fine after all. I will do a more thorough review of it to see
>     if there is a simpler way to deal with it.
> 
> 
> Have you reached a conclusion on this?
Sorry for the delay, the patch is applied now.

Thanks for reminding me,

- Felix
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel