linux-kernel - RE: Xilinx: hwicap driver comments

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20080208170829.AD23B30805D@mail3-sin.bigfish.com>
Date:	Fri, 8 Feb 2008 09:08:15 -0800
From:	"Stephen Neuendorffer" <stephen.neuendorffer@...inx.com>
To:	"Jiri Slaby" <jirislaby@...il.com>,
	"Grant Likely" <grant.likely@...retlab.ca>
Cc:	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
	"Andrew Morton" <akpm@...ux-foundation.org>
Subject: RE: Xilinx: hwicap driver comments

Missed the 'send' button on this last night.

> -----Original Message-----
> From: Jiri Slaby [mailto:jirislaby@...il.com]
> Sent: Thursday, February 07, 2008 12:09 PM
> To: Stephen Neuendorffer; Grant Likely
> Cc: Linux Kernel Mailing List; Andrew Morton
> Subject: Xilinx: hwicap driver comments
> 
> Hi,
> 
> first of all, I think that the driver should go through lkml before
upstream
> merge or at least be in -mm for a while (I think this used to be a
rule some
> time ago), correct me if I'm wrong, but none of it happened.
> 
> Few comments I have:
> - release f_op retval is silently ignored, I guess you will get your
device into
> undefined state when the first function fails (esp. when you interrupt
the sem)

Fixed.

> - semaphores are deprecated
converted to mutexes.

> - class_device_create is deprecated
Fixed.

> - module_init/exit functions should be __init, not __devinit/exit (not
a bug,
> it's subset)
Fixed.

> - this piece:
>          drvdata = kmalloc(sizeof(struct hwicap_drvdata), GFP_KERNEL);
>          if (!drvdata) {
>                  dev_err(dev, "Couldn't allocate device private
record\n");
>                  return -ENOMEM;
>          }
>   memset((void *)drvdata, 0, sizeof(struct hwicap_drvdata));
> 
>   kmalloc + memset = kzalloc
Fixed.

>   null probed_devices[id] on that fail path and on failed1 label
Fixed.
 
> - from/to (void *) casts are useless
> - io resources are at least ulong
Fixed to be resource_size_t.

> - don't understand this:
>                  memcpy(kbuf, drvdata->read_buffer, bytes_remaining);
>                  drvdata->read_buffer_in_use = bytes_remaining;
>                  free_page((unsigned long)kbuf);
Yeow.  Fixed so the sense of the memcpy correct.

> - can this overlap (=>memmove)?
>                  memcpy(drvdata->read_buffer + bytes_to_read,
>                                  drvdata->read_buffer, 4 -
bytes_to_read);
Yes, it can!  fixed.

Given the length of time that these errors have been there, I'm really
wondering if the added complexity of this buffer is worth it.

> - is platform probing function race-proof (like pci)?
Added another mutex around access to probed_devices

> - run sparse on it, you mix __user with non-__user at least
Fixed.  There is one warning (related to a function that is not called),
but I'd like to leave this in, as it should get bound to an ioctl, most
likely.

Thanks alot for the comments!

Steve


--
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/