| 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: <1386247827.11866.6.camel@oc7383187364.ibm.com>
Date: Thu, 05 Dec 2013 13:50:27 +0100
From: Frank Haverkamp <haver@...ux.vnet.ibm.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
cody@...ux.vnet.ibm.com, schwidefsky@...ibm.com,
utz.bacher@...ibm.com, mmarek@...e.cz, rmallon@...il.com,
jsvogt@...ibm.com, MIJUNG@...ibm.com, cascardo@...ux.vnet.ibm.com,
michael@...ra.de
Subject: Re: [PATCH 1/6] GenWQE PCI support, health monitoring and recovery
Hi Arnd,
Am Donnerstag, den 05.12.2013, 03:38 +0100 schrieb Arnd Bergmann:
> On Wednesday 04 December 2013, Frank Haverkamp wrote:
> > Hi Arnd & Greg,
> >
> > please let me know if my following changes are ok:
> >
> > Am Dienstag, den 03.12.2013, 15:28 +0100 schrieb Frank Haverkamp:
> >
> > > +/* Read/write from/to registers */
> > > +struct genwqe_regs_io {
> > > + __u32 num; /* register offset/address */
> > > + union {
> > > + __u64 val64;
> > > + __u32 val32;
> > > + __u16 define;
> > > + };
> > > +};
> >
> > Here I am using now:
> >
> > struct genwqe_regs_io {
> > __u64 num; /* register offset/address */
> > union {
> > __u64 val64;
> > __u32 val32;
> > __u16 define;
> > };
> > };
Changed in my next version to:
struct genwqe_reg_io {
__u64 num; /* register offset/address */
__u64 val64;
};
which is sufficient as you pointed out.
>
> This is not a bug anymore, but it seems pointless to use a union
> there rather than just a __u64 for the value.
>
> > Here I reordered and resized the members like this:
> >
> > struct genwqe_bitstream {
> > __u64 data_addr; /* pointer to image data */
> > __u32 size; /* size of image file */
> > __u32 crc; /* crc of this image */
> > __u64 target_addr; /* starting address in Flash */
> > __u32 partition; /* '0', '1', or 'v' */
> > __u32 uid; /* 1=host/x=dram */
> >
> > __u64 slu_id; /* informational/sim: SluID */
> > __u64 app_id; /* informational/sim: AppID */
> >
> > __u16 retc; /* returned from processing */
> > __u16 attn; /* attention code from processing */
> > __u32 progress; /* progress code from processing */
> > };
>
> Yes, this is fine.
>
> > > +struct genwqe_debug_data {
> > > + char driver_version[64];
> > > + __u64 slu_unitcfg;
> > > + __u64 app_unitcfg;
> > > +
> > > + __u8 ddcb_before[DDCB_LENGTH];
> > > + __u8 ddcb_prev[DDCB_LENGTH];
> > > + __u8 ddcb_finished[DDCB_LENGTH];
> > > +};
> > > +
> >
> > This I hope is ok. DDCB_LENGTH is 256.
>
> Yes.
>
> >
> > Was this already ok? My new version looks as follows:
>
> The old version was wrong.
>
> > struct genwqe_ddcb_cmd {
> > /* START of data copied to/from driver */
> > __u64 next_addr; /* chaining genwqe_ddcb_cmd */
> > __u64 flags; /* reserved */
> >
> > __u8 acfunc; /* accelerators functional unit */
> > __u8 cmd; /* command to execute */
> > __u8 asiv_length; /* used parameter length */
> > __u8 asv_length; /* length of valid return values */
> > __u16 cmdopts; /* command options */
> > __u16 retc; /* return code from processing */
>
>
> > __u16 attn; /* attention code from processing */
> > __u16 vcrc; /* variant crc16 */
> > __u32 progress; /* progress code from processing */
> >
> > __u64 deque_ts; /* dequeue time stamp */
> > __u64 cmplt_ts; /* completion time stamp */
> > __u64 disp_ts; /* SW processing start */
> >
> > /* move to end and avoid copy-back */
> > __u64 ddata_addr; /* collect debug data */
> >
> > /* command specific values */
> > __u8 asv[DDCB_ASV_LENGTH];
> >
> > /* END of data copied from driver */
> > union {
> > struct {
> > __u64 ats;
> > __u8 asiv[DDCB_ASIV_LENGTH_ATS];
> > };
> > /* used for flash update to keep it backward compatible */
> > __u8 __asiv[DDCB_ASIV_LENGTH];
> > };
> > /* END of data copied to driver */
> > };
> >
> > Trying to group the data in 64bit chunks even nicer than I had it
> > before.
>
> Yes, this works, although I would argue that it is too complex to be a nice
> interface.
>
> > > +/**
> > > + * struct genwqe_mem - Memory pinning/unpinning information
> > > + * @addr: virtual user space address
> > > + * @size: size of the area pin/dma-map/unmap
> > > + * direction: 0: read/1: read and write
> > > + *
> > > + * Avoid pinning and unpinning of memory pages dynamically. Instead
> > > + * the idea is to pin the whole buffer space required for DDCB
> > > + * opertionas in advance. The driver will reuse this pinning and the
> > > + * memory associated with it to setup the sglists for the DDCB
> > > + * requests without the need to allocate and free memory or map and
> > > + * unmap to get the DMA addresses.
> > > + *
> > > + * The inverse operation needs to be called after the pinning is not
> > > + * needed anymore. The pinnings else the pinnings will get removed
> > > + * after the device is closed. Note that pinnings will required
> > > + * memory.
> > > + */
> > > +struct genwqe_mem {
> > > + unsigned long addr;
> > > + unsigned long size;
> > > + int direction;
> > > +};
> >
> > Was wrong, as already pointed out before. It is now:
> >
> > struct genwqe_mem {
> > __u64 addr;
> > __u64 size;
> > int direction;
> > };
> >
> > I hope the int is ok here.
>
> No, it's not. The problem is that sizeof(struct genwqe_mem) is now 24 on
> most architectures (including x86-64) and 20 on x86-32.
Interesting. So int is like long architecture specific. I changed it to
be __u64 too, to avoid any problem.
> The size gets
> encoded into the ioctl number, at least after you fix this part:
>
> > > +
> > > +#define GENWQE_PIN_MEM _IOWR(GENWQE_IOC_CODE, 40, struct
> > > genwqe_mem *)
> > > +#define GENWQE_UNPIN_MEM _IOWR(GENWQE_IOC_CODE, 41, struct
> > > genwqe_mem *)
>
> ... which is also broken because sizeof(struct genwqe_mem *) is 8 on
> 64-bit and 4 on 32-bit architectures. The argument to _IOWR() is
> supposed to be struct, not a pointer. I thought we would actually
> cause a build-time warning about this bug (I wrote the code to do that)
> but I may be misremembering which bugs we can actually catch.
Ok. Yes, would actually a good thing, to save you some time helping
folks like me to get it right. Would also be nice to catch some of those
problems with automatic checking like checkpatch.pl or alike. I guess
any struct/enum in the uapi directory is in danger to be wrong, since
not automatically checked.
>
> > > +/*
> > > + * Generic synchronous DDCB execution interface.
> > > + * Synchronously execute a DDCB.
> > > + *
> > > + * Return: 0 on success or negative error code.
> > > + * -EINVAL: Invalid parameters (ASIV_LEN, ASV_LEN, illegal
> > > fixups
> > > + * no mappings found/could not create mappings
> > > + * -EFAULT: illegal addresses in fixups, purging failed
> > > + * -EBADMSG: enqueing failed, retc != DDCB_RETC_COMPLETE
> > > + */
> > > +#define GENWQE_EXECUTE_DDCB \
> > > + _IOWR(GENWQE_IOC_CODE, 50, struct genwqe_ddcb_cmd *)
> > > +
> > > +#define
> > > GENWQE_EXECUTE_RAW_DDCB \
> > > + _IOWR(GENWQE_IOC_CODE, 51, struct genwqe_ddcb_cmd *)
> > > +
> > > +/* Service Layer functions (PF only) */
> > > +#define GENWQE_SLU_UPDATE _IOWR(GENWQE_IOC_CODE, 80, struct
> > > genwqe_bistream *)
> > > +#define GENWQE_SLU_READ _IOWR(GENWQE_IOC_CODE, 81, struct
> > > genwqe_bistream *)
>
> Same bug for all of these.
>
> Arnd
>
Ok.
Thanks
Frank
--
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