Re: [LTP] [PATCH 1/2] ioctl_sg01: Skip USB devices

[Petr Vorel <pvorel@suse.cz>]

Hi Martin,

> Some USB devices write hardware info and flags to the ioctl(SG_IO)
> response buffer which results in test failure. But the info is constant
> and doesn't represent any security risk. Skip USB devices to prevent
> false positives.

> ---

> I've tested this patch on kernels v4.4 through v6.16. Non-USB generic SCSI
> block devices get correctly found and used, USB block device get skipped.

Thanks a lot for an extensive testing!

I was also verify on my machine with block device connected over USB
that it's skipped (and indeed test was blocked on master).

Tested-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Few notes below.

>  testcases/kernel/syscalls/ioctl/ioctl_sg01.c | 55 +++++++++++++++-----
>  1 file changed, 42 insertions(+), 13 deletions(-)

> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
> index fba3816c3..66ff980ce 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
> @@ -29,6 +29,9 @@
>  #include "tst_test.h"
>  #include "tst_memutils.h"

> +#define SYSDIR "/sys/block"
> +#define BLOCKDIR "/sys/block/%s/device"
> +
>  #define BUF_SIZE (128 * 4096)
>  #define CMD_SIZE 6

> @@ -38,42 +41,68 @@ static unsigned char command[CMD_SIZE];
>  static struct sg_io_hdr query;

>  /* TODO: split this off to a separate SCSI library? */
> -static const char *find_generic_scsi_device(int access_flags)
> +static const char *find_generic_scsi_device(int access_flags, int skip_usb)
>  {
> -	DIR *devdir;
> +	DIR *sysdir;
>  	struct dirent *ent;
>  	int tmpfd;
> -	static char devpath[PATH_MAX];
> +	ssize_t length;
> +	char *filename;
> +	static char devpath[PATH_MAX], syspath[PATH_MAX];

> -	errno = 0;
> -	devdir = opendir("/dev");
> +	sysdir = opendir(SYSDIR);

> -	if (!devdir)
> +	if (!sysdir)
>  		return NULL;

> -	while ((ent = SAFE_READDIR(devdir))) {
> -		/* The bug is most likely reproducible only on /dev/sg* */
> -		if (strncmp(ent->d_name, "sg", 2) || !isdigit(ent->d_name[2]))

The kernel fix was done in drivers/scsi/sg.c, it made sense to check it.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a45b599ad808

> +	/* Scan block devices */
> +	while ((ent = SAFE_READDIR(sysdir))) {
> +		if (ent->d_name[0] == '.')
> +			continue;
> +
> +		snprintf(syspath, PATH_MAX, BLOCKDIR, ent->d_name);
> +		syspath[PATH_MAX - 1] = '\0';
> +
> +		/* Real device path matches the physical HW bus path */
> +		if (!realpath(syspath, devpath))
> +			continue;
> +
> +		strncat(devpath, "/generic", PATH_MAX - strlen(devpath) - 1);

On one baremetal machine and on VM with added SCSI device this approach really
works (anything "/generic" was actually pointing to scsi_generic/sg*.

> +		devpath[PATH_MAX - 1] = '\0';
> +		length = readlink(devpath, syspath, PATH_MAX - 1);
> +
> +		if (length < 0)
> +			continue;
> +
> +		syspath[length] = '\0';
> +		filename = basename(syspath);
> +
> +		/* USB devices often return HW info in SG_IO response buffer */
> +		if (skip_usb && strstr(devpath, "/usb")) {

very nit: I would personally avoid skip_usb variable because it is always 1
(skip unconditionally). Or actually allow to set it via getopts.

Kind regards,
Petr

...
>  static void setup(void)
>  {
> -	const char *devpath = find_generic_scsi_device(O_RDONLY);
> +	const char *devpath = find_generic_scsi_device(O_RDONLY, 1);

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp