RE: [PATCH v3 01/11] PCI/P2PDMA: Support peer-to-peer memory

[David Laight]

From: Logan Gunthorpe
> Sent: 13 March 2018 23:46
...
> As Stephen pointed out, it's a requirement of the PCIe spec that a
> switch supports P2P. If you want to sell a switch that does P2P with bad
> performance then that's on you to deal with.

That surprises me (unless I missed something last time I read the spec).
While P2P writes are relatively easy to handle, reads and any other TLP that
require acks are a completely different proposition.
There are no additional fields that can be set in the read TLP and will be
reflected back in the ack(s) than can be used to route the acks back to the
correct initiator.

I'm pretty sure that to support P2P reads a switch would have to save
the received read TLP and (possibly later on) issue read TLP of its own
for the required data.
I'm not even sure it is easy to interleave the P2P reads with those
coming from the root.
That requires a potentially infinite queue of pending requests.

Some x86 root ports support P2P writes (maybe with a bios option).
It would be a shame not to be able to do P2P writes on such systems
even though P2P reads won't work.

(We looked at using P2P transfers for some data, but in the end used
a different scheme.
For our use case P2P writes were enough.
An alternative would be to access the same host memory buffer from
two different devices - but there isn't an API that lets you do that.)

David

RE: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[David Laight]

From: Stephen Kitt
> Sent: 09 March 2018 22:34
> 
> COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c.
> A number of device_handler functions use this to initialise arrays,
> and this is flagged by -Wvla.
> 
> This patch replaces COMMAND_SIZE with a variant using a formula which
> can be resolved at compile time in cases where the opcode is fixed,
> resolving the array size and avoiding the VLA. The downside is that
> the code is less maintainable and that some call sites end up having
> more complex generated code.
> 
> Since scsi_command_size_tbl is dropped, we can remove the dependency
> on BLK_SCSI_REQUEST from drivers/target/Kconfig.
> 
> This was prompted by https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Stephen Kitt 
> ---
>  block/scsi_ioctl.c |  8 
>  drivers/target/Kconfig |  1 -
>  include/scsi/scsi_common.h | 13 +++--
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 60b471f8621b..b9e176e537be 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -41,14 +41,6 @@ struct blk_cmd_filter {
> 
>  static struct blk_cmd_filter blk_default_cmd_filter;
> 
> -/* Command group 3 is reserved and should never be used.  */
> -const unsigned char scsi_command_size_tbl[8] =
> -{
> - 6, 10, 10, 12,
> - 16, 12, 10, 10
> -};
> -EXPORT_SYMBOL(scsi_command_size_tbl);
> -
>  #include 
> 
>  static int sg_get_version(int __user *p)
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index 4c44d7bed01a..b5f05b60cf06 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -4,7 +4,6 @@ menuconfig TARGET_CORE
>   depends on SCSI && BLOCK
>   select CONFIGFS_FS
>   select CRC_T10DIF
> - select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
>   select SGL_ALLOC
>   default n
>   help
> diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
> index 731ac09ed231..48d950666376 100644
> --- a/include/scsi/scsi_common.h
> +++ b/include/scsi/scsi_common.h
> @@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr)
>   return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
>  }
> 
> -extern const unsigned char scsi_command_size_tbl[8];
> -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
> +/*
> + * SCSI command sizes are as follows, in bytes, for fixed size commands, per
> + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
> + * determine its group.
> + * The size table is encoded into a 32-bit value by subtracting each value
> + * from 16, resulting in a value of 1715488362
> + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10).
> + * Command group 3 is reserved and should never be used.
> + */
> +#define COMMAND_SIZE(opcode) \
> + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)

Why not:
(6 + (15 & (0x446a6440u 

Specifying the constant in hex makes it more obvious.
It is a shame about the 16, but an offset is easier than the negate.

David

RE: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[David Laight]

From: Arnd Bergmann
> Sent: 13 February 2017 17:02
...
> >> > +   ioctl_ptr = memdup_user(arg,  _IOC_SIZE(cmd));
> >> > +   if (IS_ERR_OR_NULL(ioctl_ptr)) {
> >> > +   ret = PTR_ERR(ioctl_ptr);
> >> > +   goto out;
> >> ...
> >> > + out:
> >> > +   kfree(ioctl_ptr);
> >> > +   return ret;
...
> >> That error path doesn't look quite right to me.
...
> > good god, this is a mess this morning. Thanks for the catch, I'll review 
> > these
> > more aggressively before sending out.
> 
> memdup_user() never returns NULL, and generally speaking any use of
> IS_ERR_OR_NULL() indicates that there is something wrong with the
> interface, so aside from passing the right pointer (or NULL) into kfree()
> I think using IS_ERR() is the correct solution.

You missed the problem entirely.
Code needs to be:
if (IS_ERR(ioctl_ptr))
return PTR_ERR(ioctl_ptr);

David

RE: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[David Laight]

From: Scott Bauer Sent: 13 February 2017 16:11
> When CONFIG_KASAN is enabled, compilation fails:
> 
> block/sed-opal.c: In function 'sed_ioctl':
> block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 
> 2048 bytes [-Werror=frame-
> larger-than=]
> 
> Moved all the ioctl structures off the stack and dynamically activate
> using _IOC_SIZE()

Think I'd not that this simplifies the code considerably.
AFAICT CONFIG_KASAN is a just brainf*ck.
It at least needs annotation that copy_from_user() has properties
similar to memset().
So if the size matches that of the type then no guard space (etc)
is required.

...
> + ioctl_ptr = memdup_user(arg,  _IOC_SIZE(cmd));
> + if (IS_ERR_OR_NULL(ioctl_ptr)) {
> + ret = PTR_ERR(ioctl_ptr);
> + goto out;
...
> + out:
> + kfree(ioctl_ptr);
> + return ret;
>  }

That error path doesn't look quite right to me.

David

RE: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN

[David Laight]

From: Johannes Thumshirn
> Sent: 10 February 2017 07:46
> On 02/09/2017 06:20 PM, Scott Bauer wrote:
> > When CONFIG_KASAN is enabled, compilation fails:

Does CONFIG_KASAN allocate guard stack space around everything that
is passed by address?
That sounds completely brain-dead.
There are a lot of functions that have an 'int *' argument to return
a single value - and are never going to do anything else.

...
> > Moved all the ioctl structures off the stack and dynamically activate
> > using _IOC_SIZE()
...
> 
> > -   if (copy_from_user(, arg, sizeof(session)))
> > -   return -EFAULT;
> > -   return opal_erase_locking_range(dev, );
> > +   ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
> > +   if (!ioctl_ptr)
> > +   return -ENOMEM;
> > +   if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> > +   ret = -EFAULT;
> > +   goto out;
> > }
> 
> Can't we use memdup_user() instead of kzalloc() + copy_from_user()?

You either want the copy_from_user() or the memzero() not both.

ISTM there could be two 'library' functions, maybe:
void *get_ioctl_buf(unsigned int cmd, long arg)
to malloc the buffer, memzero/copy_from_user, returns -EFAULT if copy fails.
int put_ioctl_buf(int rval, unsigned int cmd, const void *buf)
does copy_to_user() if rval >= 0 and IOR_READ, then frees buf.
return value is rval unless the copyout fails.

David

RE: Sed-opal fixups

[David Laight]

From: Scott Bauer
> Sent: 09 February 2017 17:20
> It may be too late to change anyhting in the uapi header. When we
> switched over to using IOC_SIZE I found a bug where I had switched
> up a structure in one of the series from v4 to v5 but never changed
> the structure in the IOW. The structure that was in there was to small
> so when we kzalloc on it we don't request enough space. It worked before
> because we were using the cmd strictly as a command #, not using the IOC
> and friends.
> 
> If it's too late to modify that IOW, I can work around it by reallocing
> on the correct size for that command only. I verified the rest of the
> commands and the structures are the same.
> 
> Let me know what you think, please.

Maybe define IOC_OPAL_ACTIVATE_LSP_OLD to the incorrect value and
IOC_OPAL_ACTIVATE_LSP to the correct one.
But that relies on any users specifying the correct structure.
I wouldn't guarantee that.

At the top of the driver's ioctl path add:
if (cmd == IOC_OPAL_ACTIVATE_LSP_OLD) cmd = IOC_OPAL_ACTIVATE_LSP;

For some code I added a userspace wrapper on ioctl() to check the
size of the supplied arg matched that required by the 'cmd'.
I've also done the same in the kernel.
(all as compile time checks).

David

RE: [PATCH] block: sed-opal: reduce stack size of ioctl handler

[David Laight]

From: Arnd Bergmann
> Sent: 08 February 2017 21:15
>
> When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large 
> stack,
> as each possible ioctl argument gets its own stack area plus redzone:

Why not do a single copy_from_user() at the top of sed_ioctl() based on
the _IOC_DIR() and __IOC_SIZE() values?

Something like:
int sed_ioctl(..., unsigned int cmd, void __user *arg)
{
u64 buf[??]; /* or a union */
unsigned int cmd_sz = _IOC_SIZE(cmd);

if (_IOC_DIR(cmd) & (_IOC_WRITE | _IOC_READ) && cmd_sz > sizeof buf)
return -EINVAL;

if (_IOC_DIR(cmd) & _IOC_WRITE) {
if (copy_from_user(buf, arg, cmd_sz))
return -EFAULT;
} else {
if (IOC_DIR(cmd) & _IOC_READ))
memzero(buf, cmd_sz);
}

switch (cmd) {
...
rval = ...
...
}

if (rval >= 0 && (_IOC_DIR(cmd) & _IOC_READ)
&& copy_to_user(arg, buf, cmd_sz));
return -EFAULT;

return rval;
}

David