| 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: <1223419166.8195.37.camel@brick>
Date: Tue, 07 Oct 2008 15:39:25 -0700
From: Harvey Harrison <harvey.harrison@...il.com>
To: James Bottomley <James.Bottomley@...senPartnership.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Al Viro <viro@...IV.linux.org.uk>,
linux-arch <linux-arch@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Matthew Wilcox <matthew@....cx>,
linux-scsi <linux-scsi@...r.kernel.org>,
Boaz Harrosh <bharrosh@...asas.com>
Subject: Re: [RFC] Normalizing byteorder/unaligned access API
On Tue, 2008-10-07 at 18:12 -0400, James Bottomley wrote:
> On Tue, 2008-10-07 at 14:53 -0700, Harvey Harrison wrote:
> > [related question regarding the SCSI-private endian helper needs at the end]
> >
> > Currently on the read side, we have (le16 as an example endianness)
> >
> > le16_to_cpup(__le16 *)
> > get_unaligned_le16(void *)
> >
> > And on the write side:
> >
> > *(__le16)ptr = cpu_to_le16(u16)
> > put_unaligned_le16(u16, void *);
> >
> > On the read side, Al said he would have preferred the unaligned version
> > take the same types as the aligned, rather than void *. AKPM didn't think
> > the use of get_ was that great as get/put generally implies some kind of reference
> > taking in the kernel.
> >
> > As the le16_to_cpup has been around for so long and is more recognizable, let's
> > make it the same for the unaligned case and typesafe:
> >
> > le16_to_cpup(__le16 *)
> > unaligned_le16_to_cpup(__le16 *)
> >
> > On the write side, the above get/put and type issues are still there, in addition AKPM felt
> > that the ordering of the put_unaligned parameters was opposite what was intuitive and that
> > the pointer should come first.
> >
> > In this case, as there is currently no aligned helper (other than in some drivers defining macros)
> > define the api thusly:
> >
> > Aligned:
> > write_le16(__le16 *ptr, u16 val)
> >
> > Unaligned:
> > unaligned_write_le16(__le16 *ptr, u16 val)
>
> Well, for us it would be even worse since now we'll have to do casting
> to __beX just to shut sparse up, which makes the code even more
> unreadable.
Well, there are so many sparse warnings in scsi already, would a few
more really hurt?
To avoid the casts, you could also define some annotated, packed structs
to avoid the casting.
As an example, in the write command handling in achba.c, a patch similar to the following
(assumes the existence of a __be24 type somewhere):
Somewhere in a scsi header:
struct scsi_cmd_write6 {
u8 cmd;
__be24 lba;
u8 count;
} __packed
struct scsi_cmd_write16 {
u16 cmd;
__be64 lba;
__be32 count;
} __packed
struct scsi_cmd_write12 {
u16 cmd;
__be32 lba;
__be32 count;
u16 (harvey doesn't know scsi);
} __packed
struct scsi_cmd_write10 {
u16 cmd;
__be32 lba;
u8 (harvey doesn't know scsi);
__be16 count;
} __packed
ANd then notice that you don't need any casts even if the unaligned helpers
have strict types...that and the code gets a whole lot easier to read.
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index aa4e77c..5363a45 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1786,41 +1786,24 @@ static int aac_synchronize(struct scsi_cmnd *scsicmd)
u32 cmnd_count;
if (cmd->cmnd[0] == WRITE_6) {
- cmnd_lba = ((cmd->cmnd[1] & 0x1F) << 16) |
- (cmd->cmnd[2] << 8) |
- cmd->cmnd[3];
- cmnd_count = cmd->cmnd[4];
+ const struct scsi_cmd_write_6 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be24_to_cpup(&tmp->lba) & 0x001FFFFF;
+ cmnd_count = tmp->count;
+
if (cmnd_count == 0)
cmnd_count = 256;
} else if (cmd->cmnd[0] == WRITE_16) {
- cmnd_lba = ((u64)cmd->cmnd[2] << 56) |
- ((u64)cmd->cmnd[3] << 48) |
- ((u64)cmd->cmnd[4] << 40) |
- ((u64)cmd->cmnd[5] << 32) |
- ((u64)cmd->cmnd[6] << 24) |
- (cmd->cmnd[7] << 16) |
- (cmd->cmnd[8] << 8) |
- cmd->cmnd[9];
- cmnd_count = (cmd->cmnd[10] << 24) |
- (cmd->cmnd[11] << 16) |
- (cmd->cmnd[12] << 8) |
- cmd->cmnd[13];
+ const struct scsi_cmd_write_16 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be64_to_cpup(&tmp->lba);
+ cmnd_count = unaligned_be32_to_cpup(&tmp->count);
} else if (cmd->cmnd[0] == WRITE_12) {
- cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
- (cmd->cmnd[3] << 16) |
- (cmd->cmnd[4] << 8) |
- cmd->cmnd[5];
- cmnd_count = (cmd->cmnd[6] << 24) |
- (cmd->cmnd[7] << 16) |
- (cmd->cmnd[8] << 8) |
- cmd->cmnd[9];
+ const struct scsi_cmd_write_12 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be32_to_cpup(&tmp->lba);
+ cmnd_count = unaligned_be32_to_cpup(&tmp->count);
} else if (cmd->cmnd[0] == WRITE_10) {
- cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
- (cmd->cmnd[3] << 16) |
- (cmd->cmnd[4] << 8) |
- cmd->cmnd[5];
- cmnd_count = (cmd->cmnd[7] << 8) |
- cmd->cmnd[8];
+ const struct scsi_cmd_write_10 *tmp = cmd->cmnd;
+ cmnd_lba = unaligned_be32_to_cpup(&tmp->lba);
+ cmnd_count = unaligned_be16_to_cpup(&tmp->count);
} else
continue;
if (((cmnd_lba + cmnd_count) < lba) ||
--
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