Den 02.03.2015 13:25, skrev Dan Carpenter:
On Mon, Mar 02, 2015 at 11:54:23AM +0100, Noralf Trønnes wrote:
diff --git a/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c b/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c
[snip]
+static int lcdreg_userbuf_to_u32(const char __user *user_buf, size_t count,
+ u32 *dest, size_t dest_size)
+{
+ char *buf, *start;
+ unsigned long value;
+ int ret = 0;
+ int i;
+
+ buf = kmalloc(count, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ if (copy_from_user(buf, user_buf, count)) {
+ kfree(buf);
+ return -EFAULT;
+ }
+ buf[count] = 0;
Off-by-one overflow.
Yes
+ for (i = 0; i < count; i++)
+ if (buf[i] == ' ' || buf[i] == '\n')
+ buf[i] = 0;
+ i = 0;
+ start = buf;
+ while (start < buf + count) {
+ if (*start == 0) {
Use '\0' instead of 0.
if (*start == '\0')
Ok
+ start++;
+ continue;
+ }
+ if (i == dest_size) {
+ ret = -EFBIG;
+ break;
+ }
+ if (kstrtoul(start, 16, &value)) {
+ ret = -EINVAL;
+ break;
+ }
Consider changing this to:
ret = kstrtou32(start, 16, value);
if (ret)
break;
+ dest[i++] = value;
+ while (*start != 0)
+ start++;
This while loop is not needed because of the if statement earlier.
I missed that in my many iterations of this debugfs code.
+ };
+ kfree(buf);
+ if (ret)
+ return ret;
+
+ return i ? i : -EINVAL;
+}
+
+static ssize_t lcdreg_debugfs_write_file(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct lcdreg *reg = file->private_data;
+ int ret;
+ u32 txbuf[128];
+
+ ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
+ if (ret <= 0)
+ return -EINVAL;
This function can't return zero so preserve the error code.
if (ret < 0)
return ret;
Ok
[snip]
+static ssize_t lcdreg_debugfs_read_wr(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct lcdreg *reg = file->private_data;
+ struct lcdreg_transfer tr = {
+ .index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
+ .width = reg->debugfs_read_width,
+ .count = 1,
+ };
+ u32 txbuf[1];
+ char *buf = NULL;
+ int ret;
+
+ ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
+ if (ret != 1)
+ return -EINVAL;
+
+ tr.buf = kmalloc(lcdreg_bytes_per_word(tr.width), GFP_KERNEL);
+ if (!tr.buf)
+ return -ENOMEM;
+
+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+ lcdreg_lock(reg);
+ ret = lcdreg_read(reg, txbuf[0], &tr);
+ lcdreg_unlock(reg);
+ if (ret)
+ goto error_out;
+
+ if (!reg->debugfs_read_result) {
+ reg->debugfs_read_result = kmalloc(16, GFP_KERNEL);
+ if (!reg->debugfs_read_result) {
+ ret = -ENOMEM;
+ goto error_out;
+ }
+ }
Allocating here is strange. We only free ->debugfs_read_result on
error so it's sort of buggy as well. Also is it racy if we have
multiple readers and writers at once?
I spent some time on figuring out how to do this.
To read a register, first write the register number to the 'read' file.
Then get the result by reading the 'read' file.
So I have to keep the result somewhere, but you're right this leaks
memory. Maybe I can use devm_kmalloc here.
And yes this is racy. I'm not sure if this is a problem.
This is to be used during development or debugging, so
the user should have this under control.
But I'm open to other ways of doing this. This is the best I could
come up with after several iterations on this code.
My other solutions where worse :-)
+
+ buf = reg->debugfs_read_result;
+
+ switch (tr.width) {
+ case 8:
+ snprintf(buf, 16, "%02x\n", *(u8 *)tr.buf);
+ break;
+ case 16:
+ snprintf(buf, 16, "%04x\n", *(u16 *)tr.buf);
+ break;
+ case 24:
+ case 32:
+ snprintf(buf, 16, "%08x\n", *(u32 *)tr.buf);
+ break;
+ default:
+ ret = -EINVAL;
+ goto error_out;
+ }
+
+error_out:
+ kfree(tr.buf);
+ if (ret) {
+ kfree(reg->debugfs_read_result);
+ reg->debugfs_read_result = NULL;
+ return ret;
+ }
+
+ return count;
It's often cleaner to separate error paths from the success paths. This
is almost an excption because we free (tr.buf) on both paths. But it
would like like this, if you decide to go that way:
kfree(tr.buf);
return 0;
err_free:
kfree(reg->debugfs_read_result);
reg->debugfs_read_result = NULL;
kfree(tr.buf);
return ret;
I think it's best to stick to common patterns, and in this case the "cost"
is low. So I'll do this instead.
+static inline void lcdreg_lock(struct lcdreg *reg)
+{
+ mutex_lock(®->lock);
+}
+
+static inline void lcdreg_unlock(struct lcdreg *reg)
+{
+ mutex_unlock(®->lock);
+}
Don't add abstraction around locking. Everyone hates that. Currently
we don't have any static analysis tools which do cross function locking
analysis correctly. (I am partly to blame for that, sorry). But also
we don't want to have to look it up to see if it a mutex or a spinlock.
Then I'll surely stay away from doing this :-)
+#if defined(CONFIG_DYNAMIC_DEBUG)
+
+#define lcdreg_dbg_transfer_buf(transfer) \
+do { \
+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
+ \
+ print_hex_dump_debug(" buf=", DUMP_PREFIX_NONE, 32, \
+ groupsize, transfer->buf, len, false); \
+} while (0)
+
+#elif defined(DEBUG)
+
+#define lcdreg_dbg_transfer_buf(transfer) \
+do { \
+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
+ \
+ print_hex_dump(KERN_DEBUG, " buf=", DUMP_PREFIX_NONE, 32, \
+ groupsize, transfer->buf, len, false); \
+} while (0)
I don't understand this. Why can't we use print_hex_dump_debug() for
both? In other words:
#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
...
#else
The reason I do this is to be able to switch on/off debugging quickly
when developing.
I have dynamic debug enabled, but it's more cumbersome to use, so I
define DEBUG
when I need this output.
print_hex_dump_debug() doesn't look at DEBUG, it only cares about
DYNAMIC_DEBUG.
But, it's not suited for production code, so I guess I have to use a
script or something
to easily switch dynamic debug on/off instead.
Also can we make it an inline function?
Yes, that would be much better.
Thanks for your review Dan.
Noralf.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
