| Message ID | 20181226034254.17842-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | slirp: Use lduw_be_p in slirp_input | expand |
On Wed, 26 Dec 2018 at 03:44, Richard Henderson <richard.henderson@linaro.org> wrote: > > The pointer may be unaligned, so we must use our routines for that. > At the same time, we might as well use the big-endian version > instead of ntohs. > > This fixes sparc64 host SIGBUS during pxe boot. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > slirp/slirp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
Le mer. 26 déc. 2018 04:43, Richard Henderson <richard.henderson@linaro.org> a écrit : > The pointer may be unaligned, so we must use our routines for that. > At the same time, we might as well use the big-endian version > instead of ntohs. > > This fixes sparc64 host SIGBUS during pxe boot. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- > slirp/slirp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/slirp/slirp.c b/slirp/slirp.c > index 322edf51eb..a116f43878 100644 > --- a/slirp/slirp.c > +++ b/slirp/slirp.c > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int > pkt_len) > if (pkt_len < ETH_HLEN) > return; > > - proto = ntohs(*(uint16_t *)(pkt + 12)); > + proto = lduw_be_p(pkt + 12); > switch(proto) { > case ETH_P_ARP: > arp_input(slirp, pkt, pkt_len); > -- > 2.17.2 > > >
Hello, Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit: > The pointer may be unaligned, so we must use our routines for that. > At the same time, we might as well use the big-endian version > instead of ntohs. > > This fixes sparc64 host SIGBUS during pxe boot. I'm not at ease with applying this, when Marc-André is trying to make slirp an external library... I'd rather apply the change below, could somebody review it? Samuel slirp: Avoid unaligned 16bit memory access pkt parameter may be unaligned, so we must access it byte-wise. This fixes sparc64 host SIGBUS during pxe boot. Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> diff --git a/slirp/slirp.c b/slirp/slirp.c index ab2fc4eb8b..0e41d5aedf 100644 --- a/slirp/slirp.c +++ b/slirp/slirp.c @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) if (pkt_len < ETH_HLEN) return; - proto = ntohs(*(uint16_t *)(pkt + 12)); + proto = (((uint16_t) pkt[12]) << 8) + pkt[13]; switch(proto) { case ETH_P_ARP: arp_input(slirp, pkt, pkt_len);
On 1/17/19 10:50 AM, Samuel Thibault wrote: > Hello, > > Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit: >> The pointer may be unaligned, so we must use our routines for that. >> At the same time, we might as well use the big-endian version >> instead of ntohs. >> >> This fixes sparc64 host SIGBUS during pxe boot. > > I'm not at ease with applying this, when Marc-André is trying to make > slirp an external library... I'd rather apply the change below, could > somebody review it? Fair. > - proto = ntohs(*(uint16_t *)(pkt + 12)); > + proto = (((uint16_t) pkt[12]) << 8) + pkt[13]; This works for me too, though I note unnecessary parenthesis around the cast. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 1/17/19 12:50 AM, Samuel Thibault wrote: > Hello, > > Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit: >> The pointer may be unaligned, so we must use our routines for that. >> At the same time, we might as well use the big-endian version >> instead of ntohs. >> >> This fixes sparc64 host SIGBUS during pxe boot. > > I'm not at ease with applying this, when Marc-André is trying to make > slirp an external library... I'd rather apply the change below, could > somebody review it? > > Samuel > > > slirp: Avoid unaligned 16bit memory access > > pkt parameter may be unaligned, so we must access it byte-wise. > > This fixes sparc64 host SIGBUS during pxe boot. > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > > diff --git a/slirp/slirp.c b/slirp/slirp.c > index ab2fc4eb8b..0e41d5aedf 100644 > --- a/slirp/slirp.c > +++ b/slirp/slirp.c > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) > if (pkt_len < ETH_HLEN) > return; > > - proto = ntohs(*(uint16_t *)(pkt + 12)); > + proto = (((uint16_t) pkt[12]) << 8) + pkt[13]; > switch(proto) { > case ETH_P_ARP: > arp_input(slirp, pkt, pkt_len); What about using memcpy? -- >8 -- @@ -846,12 +846,13 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) { struct mbuf *m; - int proto; + uint16_t proto; if (pkt_len < ETH_HLEN) return; - proto = ntohs(*(uint16_t *)(pkt + 12)); + memcpy(&proto, pkt + 12, sizeof(proto)); /* Avoid unaligned 16bit access */ + proto = ntohs(proto); switch(proto) { case ETH_P_ARP: arp_input(slirp, pkt, pkt_len); ---
Philippe Mathieu-Daudé, le jeu. 17 janv. 2019 14:16:16 +0100, a ecrit: > On 1/17/19 12:50 AM, Samuel Thibault wrote: > > Hello, > > > > Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit: > >> The pointer may be unaligned, so we must use our routines for that. > >> At the same time, we might as well use the big-endian version > >> instead of ntohs. > >> > >> This fixes sparc64 host SIGBUS during pxe boot. > > > > I'm not at ease with applying this, when Marc-André is trying to make > > slirp an external library... I'd rather apply the change below, could > > somebody review it? > > > > Samuel > > > > > > slirp: Avoid unaligned 16bit memory access > > > > pkt parameter may be unaligned, so we must access it byte-wise. > > > > This fixes sparc64 host SIGBUS during pxe boot. > > > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > > > > diff --git a/slirp/slirp.c b/slirp/slirp.c > > index ab2fc4eb8b..0e41d5aedf 100644 > > --- a/slirp/slirp.c > > +++ b/slirp/slirp.c > > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) > > if (pkt_len < ETH_HLEN) > > return; > > > > - proto = ntohs(*(uint16_t *)(pkt + 12)); > > + proto = (((uint16_t) pkt[12]) << 8) + pkt[13]; > > switch(proto) { > > case ETH_P_ARP: > > arp_input(slirp, pkt, pkt_len); > > What about using memcpy? Well, it looks to me even more confusing than doing the shifts :) > -- >8 -- > @@ -846,12 +846,13 @@ static void arp_input(Slirp *slirp, const uint8_t > *pkt, int pkt_len) > void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) > { > struct mbuf *m; > - int proto; > + uint16_t proto; > > if (pkt_len < ETH_HLEN) > return; > > - proto = ntohs(*(uint16_t *)(pkt + 12)); > + memcpy(&proto, pkt + 12, sizeof(proto)); /* Avoid unaligned 16bit > access */ > + proto = ntohs(proto); > switch(proto) { > case ETH_P_ARP: > arp_input(slirp, pkt, pkt_len); > --- >
On Thu, 17 Jan 2019 at 13:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 1/17/19 12:50 AM, Samuel Thibault wrote: > > --- a/slirp/slirp.c > > +++ b/slirp/slirp.c > > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) > > if (pkt_len < ETH_HLEN) > > return; > > > > - proto = ntohs(*(uint16_t *)(pkt + 12)); > > + proto = (((uint16_t) pkt[12]) << 8) + pkt[13]; > > switch(proto) { > > case ETH_P_ARP: > > arp_input(slirp, pkt, pkt_len); > > What about using memcpy? We should use whatever the new libslirp wants to consistently use as its mechanism for loading unaligned data. I don't suppose this is the only place where it ever needs to do this. Personally I would vote for having libslirp have versions of the ld*_p functions, because they solve the problem in a clear and correct way. But that's up to Marc-André really. (As you can see from clang build logs: https://travis-ci.org/qemu/qemu/jobs/480813811 slirp/ has a lot of as-yet-unfixed "takes address of packed member" bugs, which suggest it's a bit slapdash with alignment. Running it under the clang sanitizer to look for runtime alignment errors might also be interesting.) thanks -- PMM
Hi On Thu, Jan 17, 2019 at 5:56 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 17 Jan 2019 at 13:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 1/17/19 12:50 AM, Samuel Thibault wrote: > > > --- a/slirp/slirp.c > > > +++ b/slirp/slirp.c > > > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) > > > if (pkt_len < ETH_HLEN) > > > return; > > > > > > - proto = ntohs(*(uint16_t *)(pkt + 12)); > > > + proto = (((uint16_t) pkt[12]) << 8) + pkt[13]; > > > switch(proto) { > > > case ETH_P_ARP: > > > arp_input(slirp, pkt, pkt_len); > > > > What about using memcpy? > > We should use whatever the new libslirp wants to consistently > use as its mechanism for loading unaligned data. I don't > suppose this is the only place where it ever needs to do this. > > Personally I would vote for having libslirp have versions of > the ld*_p functions, because they solve the problem in a > clear and correct way. But that's up to Marc-André really. I think I would go with a copy of qemu bswap.h, unless there is an equivalent in glib (I don't think so) or gnulib? Or other standard compiler solution. It's somehow surprising me that there is no goto solution :). > (As you can see from clang build logs: > https://travis-ci.org/qemu/qemu/jobs/480813811 > slirp/ has a lot of as-yet-unfixed "takes address of packed > member" bugs, which suggest it's a bit slapdash with > alignment. Running it under the clang sanitizer to look > for runtime alignment errors might also be interesting.) > > thanks > -- PMM -- Marc-André Lureau
Hi On Fri, Jan 18, 2019 at 3:25 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Thu, Jan 17, 2019 at 5:56 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Thu, 17 Jan 2019 at 13:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > On 1/17/19 12:50 AM, Samuel Thibault wrote: > > > > --- a/slirp/slirp.c > > > > +++ b/slirp/slirp.c > > > > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) > > > > if (pkt_len < ETH_HLEN) > > > > return; > > > > > > > > - proto = ntohs(*(uint16_t *)(pkt + 12)); > > > > + proto = (((uint16_t) pkt[12]) << 8) + pkt[13]; > > > > switch(proto) { > > > > case ETH_P_ARP: > > > > arp_input(slirp, pkt, pkt_len); > > > > > > What about using memcpy? > > > > We should use whatever the new libslirp wants to consistently > > use as its mechanism for loading unaligned data. I don't > > suppose this is the only place where it ever needs to do this. > > > > Personally I would vote for having libslirp have versions of > > the ld*_p functions, because they solve the problem in a > > clear and correct way. But that's up to Marc-André really. > > I think I would go with a copy of qemu bswap.h, unless there is an > equivalent in glib (I don't think so) or gnulib? Or other standard > compiler solution. > The GStreamer solution is also quite readable. https://gitlab.freedesktop.org/gstreamer/gstreamer/blob/master/gst/gstutils.h#L165 -- Marc-André Lureau
On Fri, 18 Jan 2019 at 11:37, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > The GStreamer solution is also quite readable. > https://gitlab.freedesktop.org/gstreamer/gstreamer/blob/master/gst/gstutils.h#L165 It has the disadvantage that you can never legitimately set GST_HAVE_UNALIGNED_ACCESS (because it is always undefined-behaviour by the C standard) and the slow-path versions are slow-and-clunky. Using malloc like the QEMU approach has the advantage of telling the compiler what you're doing so it can emit the optimal code on systems where it works. thanks -- PMM
Patchew URL: https://patchew.org/QEMU/20181226034254.17842-1-richard.henderson@linaro.org/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===
CC crypto/block.o
CC crypto/block-qcow.o
/tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
/tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
The full log is available at
http://patchew.org/logs/20181226034254.17842-1-richard.henderson@linaro.org/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
diff --git a/slirp/slirp.c b/slirp/slirp.c index 322edf51eb..a116f43878 100644 --- a/slirp/slirp.c +++ b/slirp/slirp.c @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) if (pkt_len < ETH_HLEN) return; - proto = ntohs(*(uint16_t *)(pkt + 12)); + proto = lduw_be_p(pkt + 12); switch(proto) { case ETH_P_ARP: arp_input(slirp, pkt, pkt_len);
The pointer may be unaligned, so we must use our routines for that. At the same time, we might as well use the big-endian version instead of ntohs. This fixes sparc64 host SIGBUS during pxe boot. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- slirp/slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.2