lkml.org 
[lkml]   [2006]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [patch 2/6] New Generic HW RNG
    Date
    On Sunday 07 May 2006 15:03, you wrote:
    > This does not handle the case of partial read correctly - the code
    > should be
    >
    > return ret ? : -ERESTARTSYS;
    >
    > > + if (!current_rng) {
    > > + mutex_unlock(&rng_mutex);
    > > + return -ENODEV;
    >
    > The same problem here (although finding the RNG suddenly missing after
    > we heve just read something from it is pretty unlikely).
    >
    > > + }
    > > + have_data = 0;
    > > + if (current_rng->data_present == NULL ||
    > > + current_rng->data_present(current_rng))
    > > + have_data = current_rng->data_read(current_rng, &data);
    > > + mutex_unlock(&rng_mutex);
    > > +
    > > + while (have_data && size) {
    > > + if (put_user((u8)data, buf++)) {
    > > + ret = ret ? : -EFAULT;
    > > + break;
    > > + }
    > > + size--;
    > > + ret++;
    > > + have_data--;
    > > + data>>=8;
    > > + }
    > > +
    > > + if (filp->f_flags & O_NONBLOCK)
    > > + return ret ? : -EAGAIN;
    > > +
    > > + if (need_resched()) {
    > > + schedule_timeout_interruptible(1);
    > > + } else {
    > > + err = mutex_lock_interruptible(&rng_mutex);
    > > + if (err)
    > > + return err;
    >
    > And here...
    >
    > > + if (!current_rng) {
    > > + mutex_unlock(&rng_mutex);
    > > + return -ENODEV;
    >
    > And here too.

    Whoops, will fix these.

    > > + list_for_each_entry(rng, &rng_list, list) {
    > > + if (strncmp(rng->name, buf, len) == 0) {
    >
    > This will match if the passed string is just a prefix of rng->name.
    > Apparently sysfs guarantees that the buffer passed to ->store will be
    > NUL-terminated, so this should be just a strcmp().

    I am not sure if it is guaranteed NUL terminated. Greg?
    But I will look into this.

    > > + if (rng->init) {
    > > + err = rng->init(rng);
    > > + if (err)
    > > + break;
    > > + }
    > > + if (current_rng && current_rng->cleanup)
    > > + current_rng->cleanup(current_rng);
    >
    > What if rng == current_rng here (someone has written the same RNG name
    > to the "store" attribute)? The lowlevel RNG driver should not have to
    > handle nested init/cleanup calls.

    I see. Will fix this.


    I will also fix the bcm43xx patch. It registers always with the same "name".
    That will blow up, if there is more than one bcm43xx device in the system.

    --
    Greetings Michael.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-05-07 15:11    [W:5.062 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site