Re: [patch 2/6] New Generic HW RNG

[Michael Buesch <mb@bu3sch.de>]

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.