Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user()

2020-03-05 Thread Rasmus Villemoes 
On 02/03/2020 19.31, Jann Horn wrote:
> On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko  wrote:
>> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches  wrote:
>>>
>>> So?  CONFIG_INIT_STACK_ALL by design slows down code.
>> Correct.
>>
>>> This marking would likely need to be done for nearly all
>>> 3000+ copy_from_user entries.
>> Unfortunately, yes. I was just hoping to do so for a handful of hot
>> cases that we encounter, but in the long-term a compiler solution must
>> supersede them.
>>
>>> Why not try to get something done on the compiler side
>>> to mark the function itself rather than the uses?
>> This is being worked on in the meantime as well (see
>> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
>> Do you have any particular requisitions about how this should look on
>> the source level?
> 
> Just thinking out loud: Should this be a function attribute, or should
> it be a builtin - something like __builtin_assume_initialized(ptr,
> len)? That would make it also work for macros,

But with macros (and static inlines), the compiler sees all the
initialization being done, no?

and it might simplify
> the handling of inlining in the compiler. And you wouldn't need such a
> complicated attribute that refers to function arguments by index and
> such.

Does copy_from_user guarantee to zero-initialize the remaining buffer if
copying fails partway through? Otherwise it will be hard for the
compiler to make use of an annotation such as __assume_initialized(buf,
size - ret_from_cfu) - it will have to say "ok, the caller is bailing
out unless ret_from_cfu is 0, and in that case, yes, the whole local
struct variable is indeed initialized". And we can't make the annotation
unconditionally __assume_initialized(buf, size) [unless c_f_u comes with
that guarantee] because we don't know that all callers of c_f_u() bail
out on non-zero.


Somewhat related: I've long wanted a bunch of function attributes

  __may_read(ptr, bytes)
  __may_write(ptr, bytes)
  __will_write(ptr, bytes)

The first could be used to warn about passing an uninitialized or
too-small buffer (e.g.

  struct pollfd fds[4];
  poll(fds, sizeof(fds), ...) // whoops, should have been ARRAY_SIZE)

the second also for warning about a too-small buffer, and the third
would essentially be the same as __assume_initializes. Perhaps with some
sanitization option the compiler could also instrument the function
definition to not read/write beyond the area declared via those attributes.

But the attribute syntax doesn't currently allow complex expressions in
terms of the parameter names; I'd want to annotate poll as

  int poll(struct pollfd *fds, nfds_t nfds, int to) __may_rw(fds, nfds *
sizeof(*fds))

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy

2019-09-11 Thread Rasmus Villemoes 
On 11/09/2019 11.16, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote:
>>
>>
>>> On 11 Sep 2019, at 10:52, Dan Carpenter  wrote:
>>>
>>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote:
 strcpy was used multiple times in strcpy to write into dev->name.
 I replaced them with strscpy.

Yes, that's obviously what the patch does. The commit log is supposed to
explain _why_.

 Signed-off-by: Sandro Volery 
 ---
 drivers/staging/octeon/ethernet.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

 diff --git a/drivers/staging/octeon/ethernet.c 
 b/drivers/staging/octeon/ethernet.c
 index 8889494adf1f..cf8e9a23ebf9 100644
 --- a/drivers/staging/octeon/ethernet.c
 +++ b/drivers/staging/octeon/ethernet.c
 @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED;
priv->port = CVMX_PIP_NUM_INPUT_PORTS;
priv->queue = -1;
 -strcpy(dev->name, "pow%d");
 +strscpy(dev->name, "pow%d", sizeof(dev->name));
>>>
>>> Is there a program which is generating a warning for this code?  We know
>>> that "pow%d" is 6 characters and static analysis tools can understand
>>> this code fine so we know it's safe.
>>
>> Well I was confused too but checkpatch complained about 
>> it so I figured I'd clean it up quick
> 
> Ah.  It's a new checkpatch warning.  I don't care in that case.  I'm
> fine with replacing all of these in that case.

But why? It actually gives _less_ compile-time checking (gcc and all
static tools know perfectly well what strcpy is and does, but knows
nothing of strscpy). And using sizeof() instead of ARRAY_SIZE() means a
future reader is not even sure dev->name is not just a pointer.

Moreover, it's very likely also a runtime and .text pessimization, again
because gcc knows what strcpy does, so it can just do a few immediate
stores (e.g. 0x25776f70 for the "pow%" part) instead of emitting an
actual function call.

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: exfat: Avoid use of strcpy

2019-09-11 Thread Rasmus Villemoes 
On 11/09/2019 10.41, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 07:57:49AM +0200, Sandro Volery wrote:
>> Replaced strcpy with strscpy in exfat_core.c.
>>
>> Signed-off-by: Sandro Volery 
>> ---
>>  drivers/staging/exfat/exfat_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/exfat/exfat_core.c 
>> b/drivers/staging/exfat/exfat_core.c
>> index da8c58149c35..c71b145e8a24 100644
>> --- a/drivers/staging/exfat/exfat_core.c
>> +++ b/drivers/staging/exfat/exfat_core.c
>> @@ -2964,7 +2964,7 @@ s32 resolve_path(struct inode *inode, char *path, 
>> struct chain_t *p_dir,
>>  if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE))
>>  return FFS_INVALIDPATH;
>>  
>> -strcpy(name_buf, path);
>> +strscpy(name_buf, path, sizeof(name_buf));
> 
> It checked strlen() earlier so we know that it can't overflow but, oh
> wow, the "name_buf" is a shared buffer.  wow wow wow.  This seems very
> racy.

Yeah, and note that the callers of resolve_path do seem to take a lock,
but that's not a global lock but a per-superblock (or
per-something-else) one... Obviously exfat should not rely on such a
single static buffer, but in the meantime, perhaps one should add a
name_buf_lock (though I don't know how long name_buf is actually in use,
so that needs checking).

Apart from the broken/lack of locking, it would be better to combine the
copying and length checking into a single check - i.e. do

  if (strscpy(name_buf, path, sizeof(name_buf)) < 0)
return FFS_INVALIDPATH;

That's both more efficient and fixes the open-coding of sizeof(name_buf)
that is currently used in the if().

Rasmus


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: meson: Add NULL check after the call to kmalloc()

2019-09-04 Thread Rasmus Villemoes 
On 04/09/2019 10.22, Austin Kim wrote:
> If the kmalloc() return NULL, the NULL pointer dereference will occur.
>   new_ts->ts = ts;
> 
> Add exception check after the call to kmalloc() is made.
> 
> Signed-off-by: Austin Kim 
> ---
>  drivers/staging/media/meson/vdec/vdec_helpers.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c 
> b/drivers/staging/media/meson/vdec/vdec_helpers.c
> index f16948b..e7e56d5 100644
> --- a/drivers/staging/media/meson/vdec/vdec_helpers.c
> +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
> @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, 
> u64 ts, u32 offset)
>   unsigned long flags;
>  
>   new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL);
> + if (!new_ts) {
> + dev_err(sess->core->dev, "Failed to kmalloc()\n");
> + return;
> + }
>   new_ts->ts = ts;
>   new_ts->offset = offset;

No need to printk() on error, AFAIK the mm subsystem should already make
some noise if an allocation fails.
This is not a proper fix - you need to make the function return an error
(-ENOMEM) to let the caller know allocation failed, and allow that to
propagate the error. There's only one caller, which already seems
capable of returning errors (there's an -EAGAIN), so it shouldn't be
that hard - though of course one needs to undo what has been done so far.

Also, unrelated to the kmalloc() handling: amvdec_add_ts_reorder() could
be moved to esparser.c and made static, or at the very least the
EXPORT_SYMBOL can be removed since vdec_helpers.o is linked in to the
same module as the sole user. That probably goes for all the
EXPORT_SYMBOL(amvdec_*).

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: greybus: loopback.c: remove unused lists

2018-10-24 Thread Rasmus Villemoes 
gb_loopback_device::list_op_async is never used except for the
LIST_INIT. The ::list field appears to have a few more uses, but on
closer inspection the linked list of struct gb_loopbacks that it heads
is never used for anything, so there's no reason to maintain it, much
less to keep it sorted.

Reviewed-by: Bryan O'Donoghue 
Signed-off-by: Rasmus Villemoes 
---
Sending as a proper patch. Marked v2 since this replaces earlier 2/3
and 3/3 patches. Applies on top of
b4fc4e8340784e30c5a59bf0791f9c3ce15e (staging: greybus:
loopback.c: remove unused gb_loopback::lbid).

 drivers/staging/greybus/loopback.c | 38 --
 1 file changed, 38 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c 
b/drivers/staging/greybus/loopback.c
index 7080294f705c..e4d42c1dc284 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -47,8 +47,6 @@ struct gb_loopback_device {
 
/* We need to take a lock in atomic context */
spinlock_t lock;
-   struct list_head list;
-   struct list_head list_op_async;
wait_queue_head_t wq;
 };
 
@@ -68,7 +66,6 @@ struct gb_loopback {
struct kfifo kfifo_lat;
struct mutex mutex;
struct task_struct *task;
-   struct list_head entry;
struct device *dev;
wait_queue_head_t wq;
wait_queue_head_t wq_completion;
@@ -987,37 +984,6 @@ static const struct file_operations 
gb_loopback_debugfs_latency_ops = {
.release= single_release,
 };
 
-static int gb_loopback_bus_id_compare(void *priv, struct list_head *lha,
- struct list_head *lhb)
-{
-   struct gb_loopback *a = list_entry(lha, struct gb_loopback, entry);
-   struct gb_loopback *b = list_entry(lhb, struct gb_loopback, entry);
-   struct gb_connection *ca = a->connection;
-   struct gb_connection *cb = b->connection;
-
-   if (ca->bundle->intf->interface_id < cb->bundle->intf->interface_id)
-   return -1;
-   if (cb->bundle->intf->interface_id < ca->bundle->intf->interface_id)
-   return 1;
-   if (ca->bundle->id < cb->bundle->id)
-   return -1;
-   if (cb->bundle->id < ca->bundle->id)
-   return 1;
-   if (ca->intf_cport_id < cb->intf_cport_id)
-   return -1;
-   else if (cb->intf_cport_id < ca->intf_cport_id)
-   return 1;
-
-   return 0;
-}
-
-static void gb_loopback_insert_id(struct gb_loopback *gb)
-{
-   /* perform an insertion sort */
-   list_add_tail(>entry, _dev.list);
-   list_sort(NULL, _dev.list, gb_loopback_bus_id_compare);
-}
-
 #define DEBUGFS_NAMELEN 32
 
 static int gb_loopback_probe(struct gb_bundle *bundle,
@@ -1113,7 +1079,6 @@ static int gb_loopback_probe(struct gb_bundle *bundle,
}
 
spin_lock_irqsave(_dev.lock, flags);
-   gb_loopback_insert_id(gb);
gb_dev.count++;
spin_unlock_irqrestore(_dev.lock, flags);
 
@@ -1169,7 +1134,6 @@ static void gb_loopback_disconnect(struct gb_bundle 
*bundle)
 
spin_lock_irqsave(_dev.lock, flags);
gb_dev.count--;
-   list_del(>entry);
spin_unlock_irqrestore(_dev.lock, flags);
 
device_unregister(gb->dev);
@@ -1196,8 +1160,6 @@ static int loopback_init(void)
 {
int retval;
 
-   INIT_LIST_HEAD(_dev.list);
-   INIT_LIST_HEAD(_dev.list_op_async);
spin_lock_init(_dev.lock);
gb_dev.root = debugfs_create_dir("gb_loopback", NULL);
 
-- 
2.19.1.6.gbde171bbf5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] staging: greybus: loopback.c: do insertion in O(n) instead of O(n lg n)

2018-10-23 Thread Rasmus Villemoes 
On 2018-10-11 01:03, Bryan O'Donoghue wrote:
> On 05/10/2018 15:28, Rasmus Villemoes wrote:
>> Signed-off-by: Rasmus Villemoes
>> ---
>> I have no idea if the performance matters (it probably doesn't). Feel
>> free to ignore this and the followup cleanup.
> 
> What's the problem you're fixing here ?
> 
> Is it tested ?

I got curious why one would want to keep a linked list sorted in the
first place (it can't be for doing binary searches). But it seems that
gb_loopback_device::list is unused, along with the list_op_async. Given
that the below compiles, doesn't that prove that the code is
dead/unused, or what am I missing?

diff --git a/drivers/staging/greybus/loopback.c
b/drivers/staging/greybus/loopback.c
index 7080294f705c..e4d42c1dc284 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -47,8 +47,6 @@ struct gb_loopback_device {

/* We need to take a lock in atomic context */
spinlock_t lock;
-   struct list_head list;
-   struct list_head list_op_async;
wait_queue_head_t wq;
 };

@@ -68,7 +66,6 @@ struct gb_loopback {
struct kfifo kfifo_lat;
struct mutex mutex;
struct task_struct *task;
-   struct list_head entry;
struct device *dev;
wait_queue_head_t wq;
wait_queue_head_t wq_completion;
@@ -987,37 +984,6 @@ static const struct file_operations
gb_loopback_debugfs_latency_ops = {
.release= single_release,
 };

-static int gb_loopback_bus_id_compare(void *priv, struct list_head *lha,
- struct list_head *lhb)
-{
-   struct gb_loopback *a = list_entry(lha, struct gb_loopback, entry);
-   struct gb_loopback *b = list_entry(lhb, struct gb_loopback, entry);
-   struct gb_connection *ca = a->connection;
-   struct gb_connection *cb = b->connection;
-
-   if (ca->bundle->intf->interface_id < cb->bundle->intf->interface_id)
-   return -1;
-   if (cb->bundle->intf->interface_id < ca->bundle->intf->interface_id)
-   return 1;
-   if (ca->bundle->id < cb->bundle->id)
-   return -1;
-   if (cb->bundle->id < ca->bundle->id)
-   return 1;
-   if (ca->intf_cport_id < cb->intf_cport_id)
-   return -1;
-   else if (cb->intf_cport_id < ca->intf_cport_id)
-   return 1;
-
-   return 0;
-}
-
-static void gb_loopback_insert_id(struct gb_loopback *gb)
-{
-   /* perform an insertion sort */
-   list_add_tail(>entry, _dev.list);
-   list_sort(NULL, _dev.list, gb_loopback_bus_id_compare);
-}
-
 #define DEBUGFS_NAMELEN 32

 static int gb_loopback_probe(struct gb_bundle *bundle,
@@ -1113,7 +1079,6 @@ static int gb_loopback_probe(struct gb_bundle *bundle,
}

spin_lock_irqsave(_dev.lock, flags);
-   gb_loopback_insert_id(gb);
gb_dev.count++;
spin_unlock_irqrestore(_dev.lock, flags);

@@ -1169,7 +1134,6 @@ static void gb_loopback_disconnect(struct
gb_bundle *bundle)

spin_lock_irqsave(_dev.lock, flags);
gb_dev.count--;
-   list_del(>entry);
spin_unlock_irqrestore(_dev.lock, flags);

device_unregister(gb->dev);
@@ -1196,8 +1160,6 @@ static int loopback_init(void)
 {
int retval;

-   INIT_LIST_HEAD(_dev.list);
-   INIT_LIST_HEAD(_dev.list_op_async);
spin_lock_init(_dev.lock);
gb_dev.root = debugfs_create_dir("gb_loopback", NULL);

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] staging: greybus: loopback.c: do insertion in O(n) instead of O(n lg n)

2018-10-11 Thread Rasmus Villemoes 
On 2018-10-11 01:03, Bryan O'Donoghue wrote:
> On 05/10/2018 15:28, Rasmus Villemoes wrote:
>> Signed-off-by: Rasmus Villemoes
>> ---
>> I have no idea if the performance matters (it probably doesn't). Feel
>> free to ignore this and the followup cleanup.
> 
> What's the problem you're fixing here ?

No problem, really, other than my inner Paul Hogan telling me "That's
not an insertion sort, ...".

> Is it tested ?

Compile-tested. As I said, if the performance (and inaccurate comment)
is irrelevant, just drop it.

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] staging: greybus: loopback.c: simplify prototype of gb_loopback_bus_id_compare

2018-10-05 Thread Rasmus Villemoes 
gb_loopback_bus_id_compare only has a single caller, and it no longer
needs to have a prototype compatible with being a callback for
list_sort.

Signed-off-by: Rasmus Villemoes 
---
 drivers/staging/greybus/loopback.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c 
b/drivers/staging/greybus/loopback.c
index 89c3f6fd8ddf..2c7bad66bb31 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -987,11 +987,8 @@ static const struct file_operations 
gb_loopback_debugfs_latency_ops = {
.release= single_release,
 };
 
-static int gb_loopback_bus_id_compare(void *priv, struct list_head *lha,
- struct list_head *lhb)
+static int gb_loopback_bus_id_compare(struct gb_loopback *a, struct 
gb_loopback *b)
 {
-   struct gb_loopback *a = list_entry(lha, struct gb_loopback, entry);
-   struct gb_loopback *b = list_entry(lhb, struct gb_loopback, entry);
struct gb_connection *ca = a->connection;
struct gb_connection *cb = b->connection;
 
@@ -1022,7 +1019,7 @@ static void gb_loopback_insert_id(struct gb_loopback *gb)
 * and we thus insert at the end.
 */
list_for_each_entry(gb_list, _dev.list, entry) {
-   if (gb_loopback_bus_id_compare(NULL, >entry, 
_list->entry) < 0)
+   if (gb_loopback_bus_id_compare(gb, gb_list) < 0)
break;
}
list_add_tail(>entry, _list->entry);
-- 
2.19.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging: greybus: loopback.c: remove unused gb_loopback::lbid

2018-10-05 Thread Rasmus Villemoes 
It's not obvious how the code prevents adding more than 31 elements to
the list and thus invoking undefined behaviour in the 1 << new_lbid
expression, and in practice causing ->lbid values to repeat every 32
elements.

But the definition of struct gb_loopback is local to loopback.c, and the
lbid field is entirely unused outside of this function, so it seems we
can just drop it entirely.

Signed-off-by: Rasmus Villemoes 
---
Since lbid isn't mentioned anywhere else in greybus/, it's hard to
figure out how it was meant to be used. It does seem like entirely
dead (write-only) code.

 drivers/staging/greybus/loopback.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c 
b/drivers/staging/greybus/loopback.c
index 42f6f3de967c..7080294f705c 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -97,7 +97,6 @@ struct gb_loopback {
u32 timeout_min;
u32 timeout_max;
u32 outstanding_operations_max;
-   u32 lbid;
u64 elapsed_nsecs;
u32 apbridge_latency_ts;
u32 gbphy_latency_ts;
@@ -1014,16 +1013,9 @@ static int gb_loopback_bus_id_compare(void *priv, struct 
list_head *lha,
 
 static void gb_loopback_insert_id(struct gb_loopback *gb)
 {
-   struct gb_loopback *gb_list;
-   u32 new_lbid = 0;
-
/* perform an insertion sort */
list_add_tail(>entry, _dev.list);
list_sort(NULL, _dev.list, gb_loopback_bus_id_compare);
-   list_for_each_entry(gb_list, _dev.list, entry) {
-   gb_list->lbid = 1 << new_lbid;
-   new_lbid++;
-   }
 }
 
 #define DEBUGFS_NAMELEN 32
-- 
2.19.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] staging: greybus: loopback.c: do insertion in O(n) instead of O(n lg n)

2018-10-05 Thread Rasmus Villemoes 
"Append to the list and do a merge sort" is not really an insertion
sort. While a few more lines of code, we can keep the list sorted doing
at most n comparisons by iterating until we find the first element
strictly greater than gb.

Signed-off-by: Rasmus Villemoes 
---
I have no idea if the performance matters (it probably doesn't). Feel
free to ignore this and the followup cleanup.

 drivers/staging/greybus/loopback.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c 
b/drivers/staging/greybus/loopback.c
index 7080294f705c..89c3f6fd8ddf 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -1013,9 +1013,19 @@ static int gb_loopback_bus_id_compare(void *priv, struct 
list_head *lha,
 
 static void gb_loopback_insert_id(struct gb_loopback *gb)
 {
-   /* perform an insertion sort */
-   list_add_tail(>entry, _dev.list);
-   list_sort(NULL, _dev.list, gb_loopback_bus_id_compare);
+   struct gb_loopback *gb_list;
+
+   /*
+* Keep the list sorted. Insert gb before the first element it
+* compares strictly less than. If no such element exists, the
+* loop terminates with _list->entry being _dev.list,
+* and we thus insert at the end.
+*/
+   list_for_each_entry(gb_list, _dev.list, entry) {
+   if (gb_loopback_bus_id_compare(NULL, >entry, 
_list->entry) < 0)
+   break;
+   }
+   list_add_tail(>entry, _list->entry);
 }
 
 #define DEBUGFS_NAMELEN 32
-- 
2.19.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch 09/11] x86/vdso: Simplify the invalid vclock case

2018-09-19 Thread Rasmus Villemoes 
On 2018-09-19 00:46, Thomas Gleixner wrote:
> On Tue, 18 Sep 2018, Andy Lutomirski wrote:
>>>
>>
>> Do we do better if we use signed arithmetic for the whole calculation?
>> Then a small backwards movement would result in a small backwards result.
>> Or we could offset everything so that we’d have to go back several
>> hundred ms before we cross zero.
> 
> That would be probably the better solution as signed math would be
> problematic when the resulting ns value becomes negative. As the delta is
> really small, otherwise the TSC sync check would have caught it, the caller
> should never be able to observe time going backwards.
> 
> I'll have a look into that. It needs some thought vs. the fractional part
> of the base time, but it should be not rocket science to get that
> correct. Famous last words...

Does the sentinel need to be U64_MAX? What if vgetcyc and its minions
returned gtod->cycle_last-1 (for some value of 1), and the caller just
does "if ((s64)cycles - (s64)last < 0) return fallback; ns +=
(cycles-last)* ...". That should just be a "sub ; js ; ". It's an extra
load of ->cycle_last, but only on the path where we're heading for the
fallback anyway. The value of 1 can be adjusted so that in the "js"
path, we could detect and accept an rdtsc_ordered() call that's just a
few 10s of cycles behind last and treat that as 0 and continue back on
the normal path. But maybe it's hard to get gcc to generate the expected
code.

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] android: binder: use kstrdup instead of open-coding it

2018-09-07 Thread Rasmus Villemoes 
Signed-off-by: Rasmus Villemoes 
---
 drivers/android/binder.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d58763b6b009..2abcf4501d9a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5667,12 +5667,11 @@ static int __init binder_init(void)
 * Copy the module_parameter string, because we don't want to
 * tokenize it in-place.
 */
-   device_names = kzalloc(strlen(binder_devices_param) + 1, GFP_KERNEL);
+   device_names = kstrdup(binder_devices_param, GFP_KERNEL);
if (!device_names) {
ret = -ENOMEM;
goto err_alloc_device_names_failed;
}
-   strcpy(device_names, binder_devices_param);
 
device_tmp = device_names;
while ((device_name = strsep(_tmp, ","))) {
-- 
2.16.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: Remove VLA usage

2018-03-07 Thread Rasmus Villemoes 
On Wed, Mar 07 2018, Kees Cook <keesc...@chromium.org> wrote:

> On Wed, Mar 7, 2018 at 5:10 AM, Rasmus Villemoes
> <rasmus.villem...@prevas.dk> wrote:
>> On 2018-03-07 06:46, Kees Cook wrote:
>>> The kernel would like to remove all VLA usage. This switches to a
>>> simple kasprintf() instead.
>>>
>>
>> It's probably worth pointing out that this actually fixes an
>> unconditional buffer overflow: fullname only has room for the two
>> strings and the '\n', but vsnprintf() is told that the buffer has
>> infinite size (well, INT_MAX), so there should be plenty of room to
>> append the '\0' after the '\n'.
>>
>
> Oh yes, hah. I didn't even see the \n in the string. :P
>
> So, both a VLA fix and a buffer over-run fix. Can I add your "Reviewed-by"? :)

Sure,

Reviewed-by: Rasmus Villemoes <li...@rasmusvillemoes.dk>

A nit, if you're resending anyway: can you move the "char *fullname"
declarations down a bit, to between pv,valid, and lli,rc, respectively?
That keeps the initialized and uninitialized variables nicely together
and ends up looking better.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: Remove VLA usage

2018-03-07 Thread Rasmus Villemoes 
On 2018-03-07 06:46, Kees Cook wrote:
> The kernel would like to remove all VLA usage. This switches to a
> simple kasprintf() instead.
> 
> Signed-off-by: Kees Cook 
> ---
>  drivers/staging/lustre/lustre/llite/xattr.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c 
> b/drivers/staging/lustre/lustre/llite/xattr.c
> index 532384c91447..aab4eab64289 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>   const char *name, const void *value, size_t size,
>   int flags)
>  {
> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> + char *fullname;
>   struct ll_sb_info *sbi = ll_i2sbi(inode);
>   struct ptlrpc_request *req = NULL;
>   const char *pv = value;
> @@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>   return -EPERM;
>   }
>  
> - sprintf(fullname, "%s%s\n", handler->prefix, name);

It's probably worth pointing out that this actually fixes an
unconditional buffer overflow: fullname only has room for the two
strings and the '\n', but vsnprintf() is told that the buffer has
infinite size (well, INT_MAX), so there should be plenty of room to
append the '\0' after the '\n'.

> + fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
> + if (!fullname)
> + return -ENOMEM;
>   rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
>valid, fullname, pv, size, 0, flags,
>ll_i2suppgid(inode), );
> + kfree(fullname);
>   if (rc) {
>   if (rc == -EOPNOTSUPP && handler->flags == XATTR_USER_T) {
>   LCONSOLE_INFO("Disabling user_xattr feature because it 
> is not supported on the server\n");
> @@ -364,7 +367,7 @@ static int ll_xattr_get_common(const struct xattr_handler 
> *handler,
>  struct dentry *dentry, struct inode *inode,
>  const char *name, void *buffer, size_t size)
>  {
> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> + char *fullname;
>   struct ll_sb_info *sbi = ll_i2sbi(inode);
>  #ifdef CONFIG_FS_POSIX_ACL
>   struct ll_inode_info *lli = ll_i2info(inode);
> @@ -411,9 +414,13 @@ static int ll_xattr_get_common(const struct 
> xattr_handler *handler,
>   if (handler->flags == XATTR_ACL_DEFAULT_T && !S_ISDIR(inode->i_mode))
>   return -ENODATA;
>  #endif
> - sprintf(fullname, "%s%s\n", handler->prefix, name);

Same here.

I'm a little surprised this hasn't been caugt by static analysis, I
thought gcc/coverity/smatch/whatnot had gotten pretty good at computing
the size of the output generated by a given format string with "known"
arguments and comparing to the size of the output buffer. Though of
course it does require the tool to be able to do symbolic manipulations,
in this case realizing that

outsize == strlen(x)+strlen(y)+1+1 > bufsize == strlen(x)+strlen(y)+1

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/1] staging: rtl8723bs: make memcmp() calls consistent

2017-12-13 Thread Rasmus Villemoes 
On 2017-12-13 15:49, Hans de Goede wrote:
> Hi,
> 
> On 13-12-17 12:47, Greg Kroah-Hartman wrote:
>> On Sun, Dec 10, 2017 at 08:35:12PM +0100, Nicolas Iooss wrote:
>>> rtw_pm_set() uses memcmp() with 5-chars strings and a length of 4 when
>>> parsing extra, and then parses extra+4 as an int:
>>>
>>>  if (!memcmp(extra, "lps =", 4)) {
>>>  sscanf(extra+4, "%u", );
>>>  /* ... */
>>>  } else if (!memcmp(extra, "ips =", 4)) {
>>>  sscanf(extra+4, "%u", );
>>>
>>> The space between the key ("lps" and "ips") and the equal sign seems
>>> suspicious. Remove it in order to make the calls to memcmp() consistent.
>>
>> But you now just changing the parsing logic.  What broke because of
>> this?  Did you test this codepath with your patch?
>>
>> I'm not disagreeing that this code seems really odd, but it must be
>> working as-is for someone, to change this logic will break their system
>> :(
> 
> I don't think this code can work actually, for the memcmp to
> match the extra argument must start with e.g. : "lps =" 

No, the extra argument just has to start with "lps ", so something like
"lps 1234" would "work". The memcmp call could just as well use "lps ".

but then mode
> gets read as: sscanf(extra+4, "%u", );, with extra + 4
> pointing at the "=" in the extra arg, so sscanf will stop right
> away and store 0 in mode.

See above, we don't know there's a "=" at extra+4. But in any case,
I don't think sscanf stores anything if there are no digits (and then it
would return 0 since no specifiers matched - the code also lacks a check
of the sscanf return value). But mode is initialized, so it's not going
to use some stack garbage.

All in all, some cleanup seems warranted. Why not just do a sscanf("lps
%u", ...) call and properly check the return value of that? With
whatever prefix string one thinks would be most appropriate.

> So this is for a private extension to the iw interface. I think that
> as part of the cleanup of this driver in staging we should just
> remove all private extensions, which will nicely fix the broken
> function by simply removing it :)

Yeah, that would also work...

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/5] staging: speakup: Apply format_template attribute

2017-11-12 Thread Rasmus Villemoes 
This serves as human-readable documentation as well as allowing the
format_template plugin to complain about any static initializers of this
struct member that do not have the same set of printf specifiers.

Signed-off-by: Rasmus Villemoes <li...@rasmusvillemoes.dk>
---
 drivers/staging/speakup/spk_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/spk_types.h 
b/drivers/staging/speakup/spk_types.h
index c50de6035a9a..156000c0c4d3 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -108,7 +108,7 @@ struct st_var_header {
 };
 
 struct num_var_t {
-   char *synth_fmt;
+   char *synth_fmt __format_template("%d");
int default_val;
int low;
int high;
-- 
2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/7] lib: string: add functions to case-convert strings

2016-07-07 Thread Rasmus Villemoes 
On Tue, Jul 05 2016, Markus Mayer  wrote:

> Add a collection of generic functions to convert strings to lowercase
> or uppercase.
>
> Changing the case of a string (with or without copying it first) seems
> to be a recurring requirement in the kernel that is currently being
> solved by several duplicated implementations doing the same thing. This
> change aims at reducing this code duplication.
>
> +/**
> + * strncpytoupper - Copy a length-limited string and convert to uppercase.
> + * @dst: The buffer to store the result.
> + * @src: The string to convert to uppercase.
> + * @len: Maximum string length. May be 0 to set no limit.
> + *
> + * Returns pointer to terminating '\0' in @dst.
> + */
> +char *strncpytoupper(char *dst, const char *src, size_t len)
> +{
> + size_t i;
> +
> + for (i = 0; src[i] != '\0' && (i < len || !len); i++)
> + dst[i] = toupper(src[i]);
> + if (i < len || !len)
> + dst[i] = '\0';
> +
> + return dst + i;
> +}

Hm, this seems to copy the insane semantics from strncpy of not
guaranteeing '\0'-termination.

Why use 0 as a sentinel, when (size_t)-1 == SIZE_MAX would work just as
well and require a little less code (no || !len)?

I regret suggesting this return semantics and now agree that void would
be better, especially since there doesn't seem to be anyone who can
use this (or any other) return value. How about

if (!len)
   return;

for (i = 0; i < len && src[i]; ++i)
  dst[i] = toupper(src[i]);
dst[i < len ? i : i-1] = '\0';

(I think you must do i < len before testing src[i], since the len
parameter should be an upper bound on the number of bytes to access in
both src and dst).

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] lib: string: add function strtolower()

2016-07-01 Thread Rasmus Villemoes 
On Fri, Jul 01 2016, Markus Mayer  wrote:

> Add a function called strtolower() to convert strings to lower case
> in-place, overwriting the original string.
>
> This seems to be a recurring requirement in the kernel that is
> currently being solved by several duplicated implementations doing the
> same thing.
>
> Signed-off-by: Markus Mayer 
> ---
>  include/linux/string.h |  1 +
>  lib/string.c   | 14 ++
>  2 files changed, 15 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..aad605e 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
>  #endif
>  void *memchr_inv(const void *s, int c, size_t n);
>  char *strreplace(char *s, char old, char new);
> +char *strtolower(char *s);
>  
>  extern void kfree_const(const void *x);
>  
> diff --git a/lib/string.c b/lib/string.c
> index ed83562..6e3b560 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
>   return s;
>  }
>  EXPORT_SYMBOL(strreplace);
> +
> +char *strtolower(char *s)
> +{
> + char *p;
> +
> +if (unlikely(!s))
> +return NULL;
> +
> + for (p = s; *p; p++)
> + *p = tolower(*p);
> +
> + return s;
> +}
> +EXPORT_SYMBOL(strtolower);

A few suggestions: 

- Make the function take separate src and dst parameters, making it explicitly
  allowed to pass the same value (but not other kinds of overlap, of
  course). That way one can avoid "strcpy(dst, src); strtolower(dst);".

- Drop the NULL check. If someone does "foo->bar = something;
  strtolower(foo->bar); put foo in a global data structure...", the
  dereference of foo->bar may happen much later. Doing the NULL deref
  sooner means it's much easier to find and fix the bug. (Also, other
  str* and mem* functions don't usually check for NULL).

- While it's true that strcpy and memcpy by definition return dst, that's
  mostly useless. If you want it to return anything, please make it
  something that might be used - for example, having stpcpy semantics
  (returning a pointer to dst's terminating \0) means a caller might avoid
  a strlen call.

- Maybe do strtoupper while you're at it. Quick grepping didn't find any
  use for the copy-while-lowercasing, but copy-while-uppercasing can at
  least be used in drivers/acpi/acpica/nsrepair2.c,
  drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c,
  drivers/power/power_supply_sysfs.c along with a bunch of inplace
  uppercasing.


Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: speakup: kobjects.c: fix char argument to %02x

2015-12-05 Thread Rasmus Villemoes 
If char is signed and ch happens to be negative, printing ch with
"%02x" will not do as intended (when ch is -19, one will get
"ffed"). Fix that by masking with 0xff.

Signed-off-by: Rasmus Villemoes <li...@rasmusvillemoes.dk>
---
 drivers/staging/speakup/kobjects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/kobjects.c 
b/drivers/staging/speakup/kobjects.c
index fdfeb42b2b8f..8b88f03e266b 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -567,7 +567,7 @@ ssize_t spk_var_show(struct kobject *kobj, struct 
kobj_attribute *attr,
if (ch >= ' ' && ch < '~')
*cp1++ = ch;
else
-   cp1 += sprintf(cp1, "\\x%02x", ch);
+   cp1 += sprintf(cp1, "\\x%02x", ch & 
0xff);
}
*cp1++ = '"';
*cp1++ = '\n';
-- 
2.6.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: speakup: kobjects.c: fix char argument to %02x

2015-12-05 Thread Rasmus Villemoes 
On Sun, Dec 06 2015, Joe Perches <j...@perches.com> wrote:

> On Sun, 2015-12-06 at 01:05 +0100, Rasmus Villemoes wrote:
>> If char is signed and ch happens to be negative, printing ch with
>> "%02x" will not do as intended (when ch is -19, one will get
>> "ffed"). Fix that by masking with 0xff.
>
> I presume there are a lot of these in the kernel.
> Did you use a tool to find this or just inspection?

Initially I just used coccinelle, for the most obvious candidates (with
--include-headers-for-types):

@r depends on !patch@
char c;
@@
* \( sprintf \| snprintf \| scnprintf \) (..., c, ...)

That gives lots of false positives (arguments to %c), but it's not too
bad piping to less, searching for "%[0.]2[xX]", and then checking
manually.

I'm now doing a much wider range of printf functions, but it's really
past my bedtime, so feel free to pick up the ball :-)

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: lustre: fix %.2X versus signed char issue

2015-12-05 Thread Rasmus Villemoes 
When char is signed and one of the bytes in lmm happens to have a byte
value above 127, the result of printing that with %.2X will be 8 hex
chars, the first 6 of which are 'F'. Worst case, we'll overrun our
'carefully' allocated buffer.

I didn't have the tenacity to work through the gazillion and seven
layers of macros behind CERROR, but I assume it'll all end at some
function implemented in terms of the kernel's vsnprintf. Use %*phN for
a hexdump. That'll cap the number of dumped bytes at 64. If that's a
problem, the loop could be replaced by "bin2hex(buffer, lmm,
lmm_bytes);".

Signed-off-by: Rasmus Villemoes <li...@rasmusvillemoes.dk>
---
 drivers/staging/lustre/lustre/lov/lov_pack.c | 15 +--
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_pack.c 
b/drivers/staging/lustre/lustre/lov/lov_pack.c
index 2fb1e974cc70..7f18d08929a5 100644
--- a/drivers/staging/lustre/lustre/lov/lov_pack.c
+++ b/drivers/staging/lustre/lustre/lov/lov_pack.c
@@ -258,22 +258,9 @@ static int lov_verify_lmm(void *lmm, int lmm_bytes, __u16 
*stripe_count)
int rc;
 
if (lsm_op_find(le32_to_cpu(*(__u32 *)lmm)) == NULL) {
-   char *buffer;
-   int sz;
-
CERROR("bad disk LOV MAGIC: 0x%08X; dumping LMM (size=%d):\n",
   le32_to_cpu(*(__u32 *)lmm), lmm_bytes);
-   sz = lmm_bytes * 2 + 1;
-   buffer = libcfs_kvzalloc(sz, GFP_NOFS);
-   if (buffer != NULL) {
-   int i;
-
-   for (i = 0; i < lmm_bytes; i++)
-   sprintf(buffer+2*i, "%.2X", ((char *)lmm)[i]);
-   buffer[sz - 1] = '\0';
-   CERROR("%s\n", buffer);
-   kvfree(buffer);
-   }
+   CERROR("%*phN\n", lmm_bytes, lmm);
return -EINVAL;
}
rc = lsm_op_find(le32_to_cpu(*(__u32 *)lmm))->lsm_lmm_verify(lmm,
-- 
2.6.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: android: ion: fix some format strings

2015-02-20 Thread Rasmus Villemoes 
C99 says that a precision which is simply '.' with no following digits
or * should be interpreted as 0, which means that these format strings
actually mean 'print 16 spaces'. However, the kernel's printf
implementation treats this case as if the precision was omitted. Don't
rely on that quirk.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/android/ion/ion.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index b8f1c491553e..65361ca33fc9 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -1395,7 +1395,7 @@ static int ion_debug_heap_show(struct seq_file *s, void 
*unused)
size_t total_size = 0;
size_t total_orphaned_size = 0;
 
-   seq_printf(s, %16.s %16.s %16.s\n, client, pid, size);
+   seq_printf(s, %16s %16s %16s\n, client, pid, size);
seq_puts(s, \n);
 
for (n = rb_first(dev-clients); n; n = rb_next(n)) {
@@ -1409,10 +1409,10 @@ static int ion_debug_heap_show(struct seq_file *s, void 
*unused)
char task_comm[TASK_COMM_LEN];
 
get_task_comm(task_comm, client-task);
-   seq_printf(s, %16.s %16u %16zu\n, task_comm,
+   seq_printf(s, %16s %16u %16zu\n, task_comm,
   client-pid, size);
} else {
-   seq_printf(s, %16.s %16u %16zu\n, client-name,
+   seq_printf(s, %16s %16u %16zu\n, client-name,
   client-pid, size);
}
}
@@ -1426,7 +1426,7 @@ static int ion_debug_heap_show(struct seq_file *s, void 
*unused)
continue;
total_size += buffer-size;
if (!buffer-handle_count) {
-   seq_printf(s, %16.s %16u %16zu %d %d\n,
+   seq_printf(s, %16s %16u %16zu %d %d\n,
   buffer-task_comm, buffer-pid,
   buffer-size, buffer-kmap_cnt,
   atomic_read(buffer-ref.refcount));
@@ -1435,11 +1435,11 @@ static int ion_debug_heap_show(struct seq_file *s, void 
*unused)
}
mutex_unlock(dev-buffer_lock);
seq_puts(s, \n);
-   seq_printf(s, %16.s %16zu\n, total orphaned,
+   seq_printf(s, %16s %16zu\n, total orphaned,
   total_orphaned_size);
-   seq_printf(s, %16.s %16zu\n, total , total_size);
+   seq_printf(s, %16s %16zu\n, total , total_size);
if (heap-flags  ION_HEAP_FLAG_DEFER_FREE)
-   seq_printf(s, %16.s %16zu\n, deferred free,
+   seq_printf(s, %16s %16zu\n, deferred free,
heap-free_list_size);
seq_puts(s, \n);
 
-- 
2.1.3

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8188eu: core: switch with redundant cases

2015-02-04 Thread Rasmus Villemoes 
On Wed, Feb 04 2015, Nicholas Mc Guire hof...@osadl.org wrote:

 A few redundant switch cases as well as a redundant if/else
 within one of the cases was consolidated to a single call.
 The cases are intentionally retained for documentation purposes.

[...]
 diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
 b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
 index 28918201..cd12dd7 100644
 --- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
 +++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
 @@ -484,17 +484,8 @@ void mgt_dispatcher(struct adapter *padapter, struct 
 recv_frame *precv_frame)
   /* fall through */
   case WIFI_ASSOCREQ:
   case WIFI_REASSOCREQ:
 - _mgt_dispatcher(padapter, ptable, precv_frame);
 - break;
   case WIFI_PROBEREQ:
 - if (check_fwstate(pmlmepriv, WIFI_AP_STATE))
 - _mgt_dispatcher(padapter, ptable, precv_frame);
 - else
 - _mgt_dispatcher(padapter, ptable, precv_frame);

It is highly unlikely that a function called check_fwstate has side
effects, but it might be nice checking that and making a note in the
commit log.

Rasmus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: iio: ad2s1200: Fix sign extension

2015-01-22 Thread Rasmus Villemoes 
The line above makes vel a 12-bit quantity (st-rx[] is u8). The
intention is to sign-extend vel using bit 11 as the sign bit. But
because of C's promotion rules vel = (vel  4)  4; is actually a
no-op, since vel is promoted to int before the inner
shift. sign_extend32 works equally well for 8 and 16 bits types, so
use that.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/iio/resolver/ad2s1200.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c 
b/drivers/staging/iio/resolver/ad2s1200.c
index 017d2f8379b7..c17893b4918c 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -18,6 +18,7 @@
 #include linux/delay.h
 #include linux/gpio.h
 #include linux/module.h
+#include linux/bitops.h
 
 #include linux/iio/iio.h
 #include linux/iio/sysfs.h
@@ -68,7 +69,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
break;
case IIO_ANGL_VEL:
vel = (((s16)(st-rx[0]))  4) | ((st-rx[1]  0xF0)  4);
-   vel = (vel  4)  4;
+   vel = sign_extend32(vel, 11);
*val = vel;
break;
default:
-- 
2.1.3

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: rtl8712: Remove redundant cast

2014-10-22 Thread Rasmus Villemoes 
struct firmware::data has type const u8*, as does *ppmappedfw, so the
cast to u8* is unnecessary and slightly confusing.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/rtl8712/hal_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/hal_init.c 
b/drivers/staging/rtl8712/hal_init.c
index 1d6ade0..83f9341 100644
--- a/drivers/staging/rtl8712/hal_init.c
+++ b/drivers/staging/rtl8712/hal_init.c
@@ -86,7 +86,7 @@ static u32 rtl871x_open_fw(struct _adapter *padapter, const 
u8 **ppmappedfw)
(int)padapter-fw-size);
return 0;
}
-   *ppmappedfw = (u8 *)((*praw)-data);
+   *ppmappedfw = (*praw)-data;
return (*praw)-size;
 }
 
-- 
2.0.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rtl8712: Remove redundant cast

2014-10-21 Thread Rasmus Villemoes 
struct firmware::data has type const u8*, as does *ppmappedfw, so the
cast to u8* is unnecessary and slightly confusing.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/rtl8712/hal_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/hal_init.c 
b/drivers/staging/rtl8712/hal_init.c
index 1d6ade0..cc6b390 100644
--- a/drivers/staging/rtl8712/hal_init.c
+++ b/drivers/staging/rtl8712/hal_init.c
@@ -86,7 +86,7 @@ static u32 rtl871x_open_fw(struct _adapter *padapter, const 
u8 **ppmappedfw)
(int)padapter-fw-size);
return 0;
}
-   *ppmappedfw = (u8 *)((*praw)-data);
+   *ppmappedfw = ((*praw)-data);
return (*praw)-size;
 }
 
-- 
2.0.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: vt6655: Remove redundant cast

2014-10-21 Thread Rasmus Villemoes 
Both sides have type const struct iw_handler_def*, so the cast is
unnecessary and confusing.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/vt6655/device_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index 54e16f4..9281713 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -936,7 +936,7 @@ vt6655_probe(struct pci_dev *pcid, const struct 
pci_device_id *ent)
dev-irq= pcid-irq;
dev-netdev_ops = device_netdev_ops;
 
-   dev-wireless_handlers = (struct iw_handler_def *)iwctl_handler_def;
+   dev-wireless_handlers = iwctl_handler_def;
 
rc = register_netdev(dev);
if (rc) {
-- 
2.0.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 20/22] staging: r8188eu: Replace strnicmp with strncasecmp

2014-09-16 Thread Rasmus Villemoes 
The kernel used to contain two functions for length-delimited,
case-insensitive string comparison, strnicmp with correct semantics
and a slightly buggy strncasecmp. The latter is the POSIX name, so
strnicmp was renamed to strncasecmp, and strnicmp made into a wrapper
for the new strncasecmp to avoid breaking existing users.

To allow the compat wrapper strnicmp to be removed at some point in
the future, and to avoid the extra indirection cost, do
s/strnicmp/strncasecmp/g.

Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Larry Finger larry.fin...@lwfinger.net
Cc: de...@driverdev.osuosl.org
Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/rtl8188eu/os_dep/rtw_android.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/rtw_android.c 
b/drivers/staging/rtl8188eu/os_dep/rtw_android.c
index ca2736d..2b5ddc3 100644
--- a/drivers/staging/rtl8188eu/os_dep/rtw_android.c
+++ b/drivers/staging/rtl8188eu/os_dep/rtw_android.c
@@ -79,7 +79,7 @@ int rtw_android_cmdstr_to_num(char *cmdstr)
 {
int cmd_num;
for (cmd_num = 0; cmd_num  ANDROID_WIFI_CMD_MAX; cmd_num++)
-   if (0 == strnicmp(cmdstr , android_wifi_cmd_str[cmd_num],
+   if (0 == strncasecmp(cmdstr , android_wifi_cmd_str[cmd_num],
  strlen(android_wifi_cmd_str[cmd_num])))
break;
return cmd_num;
-- 
2.0.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: omap4iss: Fix type of struct iss_device::crashed

2014-07-02 Thread Rasmus Villemoes 
The crashed member of struct iss_device is documented to be a bitmask,
but a bool doesn't hold that many (usable) bits. Lines 589 and 659 of
iss.c strongly suggest that unsigned int was meant (the same type as
struct iss_pipeline::entities). Currently, any crashed entity will be
blamed on index 0, which is unlikely to be what was intended.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/media/omap4iss/iss.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/omap4iss/iss.h 
b/drivers/staging/media/omap4iss/iss.h
index 05cd9bf..734cfee 100644
--- a/drivers/staging/media/omap4iss/iss.h
+++ b/drivers/staging/media/omap4iss/iss.h
@@ -97,7 +97,7 @@ struct iss_device {
u64 raw_dmamask;
 
struct mutex iss_mutex; /* For handling ref_count field */
-   bool crashed;
+   unsigned int crashed;
int has_context;
int ref_count;
 
-- 
1.9.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: rtl8192ee: Pass large struct by const reference

2014-07-01 Thread Rasmus Villemoes 
struct rtl_stats is rather huge (152 bytes), and since
rtl92ee_rx_command_packet() does not modify it, it might as
well be passed by const reference.

Reported by Coverity: CID 1222131

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/rtl8192ee/pci.c   | 2 +-
 drivers/staging/rtl8192ee/rtl8192ee/trx.c | 4 ++--
 drivers/staging/rtl8192ee/rtl8192ee/trx.h | 2 +-
 drivers/staging/rtl8192ee/wifi.h  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8192ee/pci.c b/drivers/staging/rtl8192ee/pci.c
index 3fe9b7b..f3abbcc 100644
--- a/drivers/staging/rtl8192ee/pci.c
+++ b/drivers/staging/rtl8192ee/pci.c
@@ -892,7 +892,7 @@ static void _rtl_pci_rx_interrupt(struct ieee80211_hw *hw)
}
 
/* handle command packet here */
-   if (rtlpriv-cfg-ops-rx_command_packet(hw, stats, skb)) {
+   if (rtlpriv-cfg-ops-rx_command_packet(hw, stats, skb)) {
dev_kfree_skb_any(skb);
goto end;
}
diff --git a/drivers/staging/rtl8192ee/rtl8192ee/trx.c 
b/drivers/staging/rtl8192ee/rtl8192ee/trx.c
index c930f52..1190c8b 100644
--- a/drivers/staging/rtl8192ee/rtl8192ee/trx.c
+++ b/drivers/staging/rtl8192ee/rtl8192ee/trx.c
@@ -1263,13 +1263,13 @@ bool rtl92ee_is_tx_desc_closed(struct ieee80211_hw *hw, 
u8 hw_queue, u16 index)
 }
 
 u32 rtl92ee_rx_command_packet(struct ieee80211_hw *hw,
- struct rtl_stats status,
+ const struct rtl_stats *status,
  struct sk_buff *skb)
 {
u32 result = 0;
struct rtl_priv *rtlpriv = rtl_priv(hw);
 
-   switch (status.packet_report_type) {
+   switch (status-packet_report_type) {
case NORMAL_RX:
result = 0;
break;
diff --git a/drivers/staging/rtl8192ee/rtl8192ee/trx.h 
b/drivers/staging/rtl8192ee/rtl8192ee/trx.h
index c2cd581..e04ee7e 100644
--- a/drivers/staging/rtl8192ee/rtl8192ee/trx.h
+++ b/drivers/staging/rtl8192ee/rtl8192ee/trx.h
@@ -872,6 +872,6 @@ void rtl92ee_tx_fill_cmddesc(struct ieee80211_hw *hw, u8 
*pdesc,
 bool b_firstseg, bool b_lastseg,
 struct sk_buff *skb);
 u32 rtl92ee_rx_command_packet(struct ieee80211_hw *hw,
- struct rtl_stats status,
+ const struct rtl_stats *status,
  struct sk_buff *skb);
 #endif
diff --git a/drivers/staging/rtl8192ee/wifi.h b/drivers/staging/rtl8192ee/wifi.h
index 96fa261..a37176a 100644
--- a/drivers/staging/rtl8192ee/wifi.h
+++ b/drivers/staging/rtl8192ee/wifi.h
@@ -1946,7 +1946,7 @@ struct rtl_hal_ops {
 u32 cmd_len, u8 *p_cmdbuffer);
bool (*get_btc_status)(void);
u32 (*rx_command_packet)(struct ieee80211_hw *hw,
-struct rtl_stats status, struct sk_buff *skb);
+const struct rtl_stats *status, struct sk_buff 
*skb);
void (*add_wowlan_pattern)(struct ieee80211_hw *hw,
   struct rtl_wow_pattern *rtl_pattern,
   u8 index);
-- 
1.9.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: rtl8821ae: Pass large struct by const reference

2014-07-01 Thread Rasmus Villemoes 
struct rtl_stats is rather huge (152 bytes), and since
rtl8812ae_rx_command_packet_handler() does not modify it, it might as
well be passed by const reference.

Reported by Coverity: CID 1167285

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/rtl8821ae/pci.c  | 2 +-
 drivers/staging/rtl8821ae/rtl8821ae/sw.c | 4 ++--
 drivers/staging/rtl8821ae/wifi.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8821ae/pci.c b/drivers/staging/rtl8821ae/pci.c
index e194ffe..f9847d1 100644
--- a/drivers/staging/rtl8821ae/pci.c
+++ b/drivers/staging/rtl8821ae/pci.c
@@ -861,7 +861,7 @@ static void _rtl_pci_rx_interrupt(struct ieee80211_hw *hw)
break;
}
 
-   rtlpriv-cfg-ops-rx_command_packet_handler(hw, status, skb);
+   rtlpriv-cfg-ops-rx_command_packet_handler(hw, status, skb);
 
/*
 *NOTICE This can not be use for mac80211,
diff --git a/drivers/staging/rtl8821ae/rtl8821ae/sw.c 
b/drivers/staging/rtl8821ae/rtl8821ae/sw.c
index 2621275..115002f 100644
--- a/drivers/staging/rtl8821ae/rtl8821ae/sw.c
+++ b/drivers/staging/rtl8821ae/rtl8821ae/sw.c
@@ -227,14 +227,14 @@ void rtl8821ae_deinit_sw_vars(struct ieee80211_hw *hw)
 
 static u32 rtl8812ae_rx_command_packet_handler(
struct ieee80211_hw *hw,
-   struct rtl_stats status,
+   const struct rtl_stats *status,
struct sk_buff *skb
)
 {
u32 result = 0;
struct rtl_priv *rtlpriv = rtl_priv(hw);
 
-   switch (status.packet_report_type) {
+   switch (status-packet_report_type) {
case NORMAL_RX:
result = 0;
break;
diff --git a/drivers/staging/rtl8821ae/wifi.h b/drivers/staging/rtl8821ae/wifi.h
index e8250da..218cd44 100644
--- a/drivers/staging/rtl8821ae/wifi.h
+++ b/drivers/staging/rtl8821ae/wifi.h
@@ -1853,7 +1853,7 @@ struct rtl_hal_ops {
 u32 cmd_len, u8 *p_cmdbuffer);
bool (*get_btc_status)(void);
u32 (*rx_command_packet_handler)(struct ieee80211_hw *hw,
-struct rtl_stats status,
+const struct rtl_stats *status,
 struct sk_buff *skb);
 };
 
-- 
1.9.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] trivial: drivers/staging/rtl8821ae/rtl8821ae: Fix closing brace followed by if

2014-06-20 Thread Rasmus Villemoes 
Greg Kroah-Hartman gre...@linuxfoundation.org writes:

 On Fri, Jun 20, 2014 at 09:56:35PM +0200, Rasmus Villemoes wrote:
 All of the code is #if 0'd out, and the change just replaces a
 space with a newline, so this obviously doesn't change anything.

 How about just deleting all the #if 0 code out entirely so no one has to
 worry about it anymore?

Fine by me. Like this?

From: Rasmus Villemoes li...@rasmusvillemoes.dk
Subject: [PATCH] drivers/staging/rtl8821ae/rtl8821ae: Remove dead code

This is all #if 0'ed out, and it contains some rather weird stuff
(post-increment of a bool, for example). Nuke it.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c 
b/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c
index 7b1d113..25e751b 100644
--- a/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c
+++ b/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c
@@ -1706,20 +1706,6 @@ void rtl8821ae_dm_bt_inq_page_monitor(struct 
ieee80211_hw *hw)
rtlpcipriv-btcoexist.current_state =~ 
BT_COEX_STATE_BT_INQ_PAGE;
}
}
-
-#if 0
-   if (hal_coex_8821ae.b_c2h_bt_inquiry_page) {
-   hal_coex_8821ae.b_c2h_bt_inquiry_page++;
-   // bt inquiry or page is started.
-   } if(hal_coex_8821ae.b_c2h_bt_inquiry_page) {
-   rtlpcipriv-btcoexist.current_state |= 
BT_COEX_STATE_BT_INQ_PAGE;
-   if(hal_coex_8821ae.bt_inquiry_page_cnt = 4)
-   hal_coex_8821ae.bt_inquiry_page_cnt = 0;
-   hal_coex_8821ae.bt_inquiry_page_cnt++;
-   } else {
-   rtlpcipriv-btcoexist.current_state =~ 
BT_COEX_STATE_BT_INQ_PAGE;
-   }
-#endif
 }
 
 void rtl8821ae_dm_bt_reset_action_profile_state(struct ieee80211_hw *hw)
-- 
1.9.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] drivers/staging/rtl8821ae/rtl8821ae: Remove dead code

2014-06-20 Thread Rasmus Villemoes 
This is all #if 0'ed out, and it contains some rather weird stuff
(post-increment of a bool, for example). Nuke it.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
---
 drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c 
b/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c
index 7b1d113..25e751b 100644
--- a/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c
+++ b/drivers/staging/rtl8821ae/rtl8821ae/hal_btc.c
@@ -1706,20 +1706,6 @@ void rtl8821ae_dm_bt_inq_page_monitor(struct 
ieee80211_hw *hw)
rtlpcipriv-btcoexist.current_state =~ 
BT_COEX_STATE_BT_INQ_PAGE;
}
}
-
-#if 0
-   if (hal_coex_8821ae.b_c2h_bt_inquiry_page) {
-   hal_coex_8821ae.b_c2h_bt_inquiry_page++;
-   // bt inquiry or page is started.
-   } if(hal_coex_8821ae.b_c2h_bt_inquiry_page) {
-   rtlpcipriv-btcoexist.current_state |= 
BT_COEX_STATE_BT_INQ_PAGE;
-   if(hal_coex_8821ae.bt_inquiry_page_cnt = 4)
-   hal_coex_8821ae.bt_inquiry_page_cnt = 0;
-   hal_coex_8821ae.bt_inquiry_page_cnt++;
-   } else {
-   rtlpcipriv-btcoexist.current_state =~ 
BT_COEX_STATE_BT_INQ_PAGE;
-   }
-#endif
 }
 
 void rtl8821ae_dm_bt_reset_action_profile_state(struct ieee80211_hw *hw)
-- 
1.9.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel