linux-kernel - Re: [patch 3/3] ps3: FLASH ROM Storage Driver

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <Pine.LNX.4.62.0707191106310.8661@pademelon.sonytel.be>
Date:	Thu, 19 Jul 2007 13:25:47 +0200 (CEST)
From:	Geert Uytterhoeven <Geert.Uytterhoeven@...ycom.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
cc:	Andy Whitcroft <apw@...dowen.org>, Jens Axboe <axboe@...nel.dk>,
	"James E.J. Bottomley" <James.Bottomley@...elEye.com>,
	linux-scsi@...r.kernel.org,
	Linux Kernel Development <linux-kernel@...r.kernel.org>,
	Linux/PPC Development <linuxppc-dev@...abs.org>,
	Paul Mackerras <paulus@...ba.org>
Subject: Re: [patch 3/3] ps3: FLASH ROM Storage Driver

	Hi Andrew,

On Wed, 18 Jul 2007, Andrew Morton wrote:
> > +struct ps3flash_private {
> > +	struct mutex mutex;	/* Bounce buffer mutex */
> > +};
> > +#define ps3flash_priv(dev)	((dev)->sbd.core.driver_data)
> 
> bzzzt!

Same defense as ps3rom ;-)

> > +static loff_t ps3flash_llseek(struct file *file, loff_t offset, int origin)
> > +{
> > +	struct ps3_storage_device *dev = ps3flash_dev;
> > +	u64 size = dev->regions[dev->region_idx].size*dev->blk_size;
> > +
> > +	switch (origin) {
> > +	case 1:
> > +		offset += file->f_pos;
> > +		break;
> > +	case 2:
> > +		offset += size;
> > +		break;
> > +	}
> > +	if (offset < 0)
> > +		return -EINVAL;
> > +
> > +	file->f_pos = offset;
> > +	return file->f_pos;
> > +}
> 
> lseek implementations usually need locking.  file->f_mapping->host->i_mutex
> would be typical.
> 
> That locking mostly protects 64-bit f_pos on 32-bit architectures so you
> can perahps kinda get away with it here.  However the code is a bit racy
> even on 64-bit, for silly userspace.
> 
> That being said, I'd have thought that you could use one of our many
> generic lseek library fucntions here, or even an newly-exported
> block_llseek().  That assumes that i_size is correct, which I trust is the
> case.

You mean generic_file_llseek()?

Who is responsible for setting i_size in that case? This is not a file, but a
character device, so currently i_size is always zero.

So I'll just add the missing locks.

> > +static ssize_t ps3flash_read(struct file *file, char __user *buf, size_t count,
> > +			     loff_t *pos)
> > +{
> > +	struct ps3_storage_device *dev = ps3flash_dev;
> > +	struct ps3flash_private *priv = ps3flash_priv(dev);
> > +	u64 size, start_sector, end_sector, offset;
> > +	ssize_t sectors_read;
> > +	size_t remaining, n;
> > +
> > +	dev_dbg(&dev->sbd.core,
> > +		"%s:%u: Reading %zu bytes at position %lld to user 0x%p\n",
> > +		__func__, __LINE__, count, *pos, buf);
> > +
> > +	size = dev->regions[dev->region_idx].size*dev->blk_size;
> > +	if (*pos >= size || !count)
> > +		return 0;
> > +
> > +	if (*pos+count > size) {
> > +		dev_dbg(&dev->sbd.core,
> > +			"%s:%u Truncating count from %zu to %llu\n", __func__,
> > +			__LINE__, count, size - *pos);
> > +		count = size - *pos;
> > +	}
> > +
> > +	start_sector = do_div_llr(*pos, dev->blk_size, &offset);
> > +	end_sector = DIV_ROUND_UP(*pos+count, dev->blk_size);
> > +
> > +	remaining = count;
> > +	do {
> > +		mutex_lock(&priv->mutex);
> > +
> > +		sectors_read = ps3flash_read_sectors(dev, start_sector,
> > +						     end_sector-start_sector,
> > +						     0);
> > +		if (sectors_read < 0) {
> > +			mutex_unlock(&priv->mutex);
> > +			return sectors_read;
> > +		}
> > +
> > +		n = min(remaining, sectors_read*dev->blk_size-offset);
> > +		dev_dbg(&dev->sbd.core,
> > +			"%s:%u: copy %lu bytes from 0x%p to user 0x%p\n",
> > +			__func__, __LINE__, n, dev->bounce_buf+offset, buf);
> > +		if (copy_to_user(buf, dev->bounce_buf+offset, n)) {
> > +			mutex_unlock(&priv->mutex);
> > +			return -EFAULT;
> > +		}
> > +
> > +		mutex_unlock(&priv->mutex);
> > +
> > +		*pos += n;
> > +		buf += n;
> > +		remaining -= n;
> > +		start_sector += sectors_read;
> > +		offset = 0;
> > +	} while (remaining > 0);
> > +
> > +	return count;
> > +}
> 
> There are several nasty deeply-embedded return points in this function.

Will change to `goto fail' and common error return.

> > +	if (*pos+count > size) {
> 
> checkpatch missed stuff here.

Checkpatch is silent...

> > +		dev_dbg(&dev->sbd.core,
> > +			"%s:%u Truncating count from %zu to %llu\n", __func__,
> > +			__LINE__, count, size - *pos);
> > +		count = size - *pos;
> > +	}
> > +
> > +	chunk_sectors = dev->bounce_size / dev->blk_size;
> > +
> > +	start_write_sector = do_div_llr(*pos, dev->bounce_size, &offset) *
> > +			     chunk_sectors;
> 
> It's strange to see a do_div_llr() in the middle of all this 64-bit-only
> code.  Could use / and %?

Sure.

Note that in theory do_div_llr() could do the division and modulo in one
instruction on architectures that support that (hmm, that's div_long_long_rem).

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@...ycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619