[PATCH] bcache: avoid a dangerous addressing in closure_queue

[PATCH] bcache: avoid a dangerous addressing in closure_queue

[Liang Chen]

The use of the union reduces the size of closure struct by taking advantage
of the current size of its members. The offset of func in work_struct equals
the size of the first three members, so that work.work_func will just
reference the forth member - the pointer to closure_fn.

This is smart but dangerous. It can be broken if work_struct or the other
ones get changed, and can be a bit difficult to debug.

Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
---
 drivers/md/bcache/closure.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
index 1ec84ca..665c470 100644
--- a/drivers/md/bcache/closure.h
+++ b/drivers/md/bcache/closure.h
@@ -251,8 +251,9 @@ static inline void set_closure_fn(struct closure *cl, closure_fn *fn,
 static inline void closure_queue(struct closure *cl)
 {
 	struct workqueue_struct *wq = cl->wq;
+	closure_fn		*fn = cl->fn;
 	if (wq) {
-		INIT_WORK(&cl->work, cl->work.func);
+		INIT_WORK(&cl->work, (work_func_t)fn);
 		BUG_ON(!queue_work(wq, &cl->work));
 	} else
 		cl->fn(cl);
-- 
1.8.3.1

Re: [PATCH] bcache: avoid a dangerous addressing in closure_queue

[Christoph Hellwig]

On Wed, Jul 05, 2017 at 08:53:19PM +0800, Liang Chen wrote:
> The use of the union reduces the size of closure struct by taking advantage
> of the current size of its members. The offset of func in work_struct equals
> the size of the first three members, so that work.work_func will just
> reference the forth member - the pointer to closure_fn.
> 
> This is smart but dangerous. It can be broken if work_struct or the other
> ones get changed, and can be a bit difficult to debug.

Please, don't ever cast function pointers, as that's extremely
dangerous.

Re: [PATCH] bcache: avoid a dangerous addressing in closure_queue

[Liang Chen]

I had the same feeling, and was reluctant to do so. The reason for making this
change was that current code implicitly converts work_func_t to closure_fn, and
it also depends on the offset and size of a few struct not being changed. So the
patch was introduced essentially to solve that, and keep the impact small
meanwhile.

But as you pointed out, explicit casting is still bad too (was hoping it can be
considered less bad at this situation). I will think of a better idea
to handle this
issue unless people agree that the current behaviour is safe.

Re: [PATCH] bcache: avoid a dangerous addressing in closure_queue

[Christoph Hellwig]

On Thu, Jul 06, 2017 at 08:18:02AM +0800, Liang Chen wrote:
> But as you pointed out, explicit casting is still bad too (was hoping it can be
> considered less bad at this situation). I will think of a better idea
> to handle this
> issue unless people agree that the current behaviour is safe.

Yes, let's find a way to avoids both the implicit and the explicit cast.