| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4927EA7D.7090003@s5r6.in-berlin.de>
Date: Sat, 22 Nov 2008 12:18:21 +0100
From: Stefan Richter <stefanr@...6.in-berlin.de>
To: Harvey Harrison <harvey.harrison@...il.com>
CC: LKML <linux-kernel@...r.kernel.org>,
Al Viro <viro@...IV.linux.org.uk>,
linux1394-devel@...ts.sourceforge.net
Subject: Re: [RFC-PATCH] ieee1394: endian annotations of drivers/ieee1394
Stefan Richter wrote:
> Harvey Harrison wrote:
>> Annotations are mostly trivial, quadlet_t -> __be32, octlet_t -> __be64
>> which will have no effect on compiled code.
>
> I haven't looked at it yet but have point out one important thing:
> This is one of the many issues in drivers/ieee1394 which have already
> been fixed by introducing drivers/firewire. I appreciate the work which
> you put into this, but I don't know yet if we want to take it in.
I test-applied and test-compiled it on x86-32 now. There are still a
number of warnings from sparse after applying it:
- in the ieee1394 core's transaction code, among else due to how
packet headers are handled by the core and the low-level drivers,
- in ohci1394, when accessing self-IDs and DMA programs,
- in raw1394 when dealing with userspace data,
- in eth1394, e.g. due to in-place endian conversions and because
eth1394 uses bitfield datatypes for on-the-wire data.
I don't think that any of those are exceedingly difficult to fix, but
they are not entirely trivial, require time for development, testing and
review, and bring a risk of regressions.
As long as not *all* endianess related warnings from sparse go away in
drivers/ieee1394/, there is no benefit from starting with endianess
annotations in drivers/ieee1394/ in the first place. Because there is
already a way to compile an IEEE 1394 capable Linux kernel which is
sparse-clean:
CONFIG_FIREWIRE=m
# CONFIG_IEEE1394 is not set
So, our time should better be spent on fixing up drivers/firewire/ to
make it the full replacement of drivers/ieee1394/ which it is intended
to be, see http://ieee1394.wiki.kernel.org/index.php/Juju_Migration and
http://ieee1394.wiki.kernel.org/index.php/To_Do .
Nevertheless, some comments to this patch:
>> Changed the protoypes of the read_regs, write_regs, lock_regs, lock64_regs
>> to take BE values rather than quadlet/octlet...propagated this through
>> helper functions.
More changes like this (comparably simple) in-kernel API change would be
required to finish the work.
>> csr.c: work directly with the BE values in the lock functions and only
>> convert to cpu-endian on demand, this part is non-trivial, but pretty
>> easy to verify. Introduce a few temporary variables to make in clear
>> we are in cpu-endianness in a few cases. Also, remove a few macros
>> that were used only once and obscured what was actually happening when
>> setting the type and generation...this eliminated some gratuitous
>> byteswapping back and forth between cpu and big endian.
OK, although these temporary variables aren't exactly beautiful.
It could be that switching more of the csr.h::struct csr_control members
to big endian would result in nicer code.
>> sbp.c: Eliminate the in-place be32-to-cpu inline which was only used once
>> and do the conversion once in the places where each struct member is set/read,
>> this propagates into one helper function that also now works directly in
>> be32.
>>
>> Also eliminate a few of the calls to the cpu-to-be32 that swapped a constant
>> 8 bytes and set the struct members directly as be32 values.
OK. Incidentally, these are fine examples of "already fixed in
drivers/firewire/fw-sbp2.c".
>> dv1394.c is the only place where a change in behavior is intentional, the
>> setting of the flags is done as cpu-endian where everwhere else it is done
>> as little-endian, see the potion of the diff in ir_tasklet_func()
An endianess bug fix would have to be done as a separate patch.
>> The rest of the changes end up being pretty simple annotations of struct
>> members that were always treated as a particular endianness, mark them as
>> such.
>>
>> Signed-off-by: Harvey Harrison <harvey.harrison@...il.com>
>> ---
>> drivers/ieee1394/csr.c | 148 ++++++++++++++++--------------
>> drivers/ieee1394/csr.h | 14 ++--
>> drivers/ieee1394/csr1212.c | 55 +++++-------
>> drivers/ieee1394/csr1212.h | 14 ++--
>> drivers/ieee1394/dv1394-private.h | 44 +++++-----
>> drivers/ieee1394/dv1394.c | 12 +-
>> drivers/ieee1394/eth1394.c | 30 +++---
>> drivers/ieee1394/eth1394.h | 12 +-
>> drivers/ieee1394/highlevel.c | 8 +-
>> drivers/ieee1394/highlevel.h | 20 ++--
>> drivers/ieee1394/hosts.c | 4 +-
>> drivers/ieee1394/hosts.h | 2 +-
>> drivers/ieee1394/ieee1394_core.h | 2 +-
>> drivers/ieee1394/ieee1394_transactions.c | 8 +-
>> drivers/ieee1394/ieee1394_transactions.h | 6 +-
>> drivers/ieee1394/nodemgr.c | 14 ++--
>> drivers/ieee1394/nodemgr.h | 2 +-
>> drivers/ieee1394/ohci1394.c | 2 +-
>> drivers/ieee1394/ohci1394.h | 8 +-
>> drivers/ieee1394/pcilynx.h | 2 +-
>> drivers/ieee1394/raw1394.c | 42 ++++----
>> drivers/ieee1394/sbp2.c | 119 ++++++++++--------------
>> drivers/ieee1394/sbp2.h | 10 +-
>> drivers/ieee1394/video1394.c | 2 +-
>> 24 files changed, 278 insertions(+), 302 deletions(-)
>>
[...]
>> --- a/drivers/ieee1394/ieee1394_core.h
>> +++ b/drivers/ieee1394/ieee1394_core.h
>> @@ -69,7 +69,7 @@ struct hpsb_packet {
>> size_t header_size; /* as filled in, not counting the CRC */
>>
>> /* Buffers */
>> - quadlet_t *data; /* can be DMA-mapped */
>> + __be32 *data; /* can be DMA-mapped */
>> quadlet_t header[5];
>> quadlet_t embedded_data[0]; /* keep as last member */
>> };
__be32 embedded_data[0];
[...]
>> --- a/drivers/ieee1394/ieee1394_transactions.c
>> +++ b/drivers/ieee1394/ieee1394_transactions.c
>> @@ -60,7 +60,7 @@ static void fill_async_readblock(struct hpsb_packet *packet, u64 addr,
>> }
>>
>> static void fill_async_writequad(struct hpsb_packet *packet, u64 addr,
>> - quadlet_t data)
>> + __be32 data)
>> {
>> PREP_ASYNC_HEAD_ADDRESS(TCODE_WRITEQ);
>> packet->header[3] = data;
Wrong, packet->header[] is currently CPU-endian.
(As a rule of thumb, if a sparse annotation patch adds new sparse
warnings, further investigation is called for.)
[...]
>> --- a/drivers/ieee1394/ieee1394_transactions.h
>> +++ b/drivers/ieee1394/ieee1394_transactions.h
[...]
>> int hpsb_packet_success(struct hpsb_packet *packet);
>> int hpsb_read(struct hpsb_host *host, nodeid_t node, unsigned int generation,
>> - u64 addr, quadlet_t *buffer, size_t length);
>> + u64 addr, __be32 *buffer, size_t length);
>> int hpsb_write(struct hpsb_host *host, nodeid_t node, unsigned int generation,
>> - u64 addr, quadlet_t *buffer, size_t length);
>> + u64 addr, __be32 *buffer, size_t length);
>> int hpsb_lock(struct hpsb_host *host, nodeid_t node, unsigned int generation,
>> u64 addr, int extcode, quadlet_t *data, quadlet_t arg);
You generated the diff against code which contains the new firedtv DVB
driver but did not annotate the new ieee394 core code introduced for
firedtv and the ieee1394-interfacing firedtv code. Doing the ieee1394
annotations in parallel with the ongoing work on the new firedtv driver
(instead of doing it sequentially) would be another unnecessary sink of
developer and maintainer time...
--
Stefan Richter
-=====-==--- =-== =-==-
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists