Re: [PATCH] Revert "async: simplify lowest_in_progress()"

[Tejun Heo]

On Mon, Nov 20, 2017 at 11:51:47PM +0100, Rasmus Villemoes wrote:
> This reverts commit 92266d6ef60c2381c980c6cdcb2a5c1667b36b49, which
> was simply wrong: In the case where domain is NULL, we now use the
> wrong offsetof() in the list_first_entry macro, so we don't actually
> fetch the ->cookie value, but rather the eight bytes located
> sizeof(struct list_head) further into the struct async_entry.
> 
> On 64 bit, that's the data member, while on 32 bit, we get a u64 built
> from func and data in some order.
> 
> I think the bug happens to be harmless in practice: It obviously only
> affects callers which pass a NULL domain, and AFAICT the only such
> caller is
> 
>   async_synchronize_full() ->
>   async_synchronize_full_domain(NULL) ->
>   async_synchronize_cookie_domain(ASYNC_COOKIE_MAX, NULL)
> 
> and the ASYNC_COOKIE_MAX means that in practice we end up waiting for
> the async_global_pending list to be empty - but it would break if
> somebody happened to pass (void*)-1 as the data element to
> async_schedule, and of course also if somebody ever does a
> async_synchronize_cookie_domain(, NULL) with a "finite" cookie value.
> 
> Cc: sta...@vger.kernel.org # 3.10+
> Signed-off-by: Rasmus Villemoes 

Ughh... indeed.

Acked-by: Tejun Heo 

Sorry about that.  Can you please resend the patch w/ Andrew Morton
cc'd?  I think it'd be best to route this through -mm.

Thanks.

-- 
tejun

Re: [PATCH] Revert "async: simplify lowest_in_progress()"

[Tejun Heo]

On Mon, Nov 20, 2017 at 11:51:47PM +0100, Rasmus Villemoes wrote:
> This reverts commit 92266d6ef60c2381c980c6cdcb2a5c1667b36b49, which
> was simply wrong: In the case where domain is NULL, we now use the
> wrong offsetof() in the list_first_entry macro, so we don't actually
> fetch the ->cookie value, but rather the eight bytes located
> sizeof(struct list_head) further into the struct async_entry.
> 
> On 64 bit, that's the data member, while on 32 bit, we get a u64 built
> from func and data in some order.
> 
> I think the bug happens to be harmless in practice: It obviously only
> affects callers which pass a NULL domain, and AFAICT the only such
> caller is
> 
>   async_synchronize_full() ->
>   async_synchronize_full_domain(NULL) ->
>   async_synchronize_cookie_domain(ASYNC_COOKIE_MAX, NULL)
> 
> and the ASYNC_COOKIE_MAX means that in practice we end up waiting for
> the async_global_pending list to be empty - but it would break if
> somebody happened to pass (void*)-1 as the data element to
> async_schedule, and of course also if somebody ever does a
> async_synchronize_cookie_domain(, NULL) with a "finite" cookie value.
> 
> Cc: sta...@vger.kernel.org # 3.10+
> Signed-off-by: Rasmus Villemoes 

Ughh... indeed.

Acked-by: Tejun Heo 

Sorry about that.  Can you please resend the patch w/ Andrew Morton
cc'd?  I think it'd be best to route this through -mm.

Thanks.

-- 
tejun

Re: [PATCH] Revert "async: simplify lowest_in_progress()"

[Adam Wallis]

On 11/20/2017 5:51 PM, Rasmus Villemoes wrote:
> This reverts commit 92266d6ef60c2381c980c6cdcb2a5c1667b36b49, which
> was simply wrong: In the case where domain is NULL, we now use the
> wrong offsetof() in the list_first_entry macro, so we don't actually
> fetch the ->cookie value, but rather the eight bytes located
> sizeof(struct list_head) further into the struct async_entry.
> 
> On 64 bit, that's the data member, while on 32 bit, we get a u64 built
> from func and data in some order.
> 
> I think the bug happens to be harmless in practice: It obviously only
> affects callers which pass a NULL domain, and AFAICT the only such
> caller is
> 
>   async_synchronize_full() ->
>   async_synchronize_full_domain(NULL) ->
>   async_synchronize_cookie_domain(ASYNC_COOKIE_MAX, NULL)
> 
> and the ASYNC_COOKIE_MAX means that in practice we end up waiting for
> the async_global_pending list to be empty - but it would break if
> somebody happened to pass (void*)-1 as the data element to
> async_schedule, and of course also if somebody ever does a
> async_synchronize_cookie_domain(, NULL) with a "finite" cookie value.
> 
> Cc: sta...@vger.kernel.org # 3.10+

Recommend adding "Fixes" notation here referencing the original broken commit.

> Signed-off-by: Rasmus Villemoes 
> ---
[..]

-- 
Adam Wallis
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] Revert "async: simplify lowest_in_progress()"

[Adam Wallis]

On 11/20/2017 5:51 PM, Rasmus Villemoes wrote:
> This reverts commit 92266d6ef60c2381c980c6cdcb2a5c1667b36b49, which
> was simply wrong: In the case where domain is NULL, we now use the
> wrong offsetof() in the list_first_entry macro, so we don't actually
> fetch the ->cookie value, but rather the eight bytes located
> sizeof(struct list_head) further into the struct async_entry.
> 
> On 64 bit, that's the data member, while on 32 bit, we get a u64 built
> from func and data in some order.
> 
> I think the bug happens to be harmless in practice: It obviously only
> affects callers which pass a NULL domain, and AFAICT the only such
> caller is
> 
>   async_synchronize_full() ->
>   async_synchronize_full_domain(NULL) ->
>   async_synchronize_cookie_domain(ASYNC_COOKIE_MAX, NULL)
> 
> and the ASYNC_COOKIE_MAX means that in practice we end up waiting for
> the async_global_pending list to be empty - but it would break if
> somebody happened to pass (void*)-1 as the data element to
> async_schedule, and of course also if somebody ever does a
> async_synchronize_cookie_domain(, NULL) with a "finite" cookie value.
> 
> Cc: sta...@vger.kernel.org # 3.10+

Recommend adding "Fixes" notation here referencing the original broken commit.

> Signed-off-by: Rasmus Villemoes 
> ---
[..]

-- 
Adam Wallis
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH] Revert "async: simplify lowest_in_progress()"

[Rasmus Villemoes]

This reverts commit 92266d6ef60c2381c980c6cdcb2a5c1667b36b49, which
was simply wrong: In the case where domain is NULL, we now use the
wrong offsetof() in the list_first_entry macro, so we don't actually
fetch the ->cookie value, but rather the eight bytes located
sizeof(struct list_head) further into the struct async_entry.

On 64 bit, that's the data member, while on 32 bit, we get a u64 built
from func and data in some order.

I think the bug happens to be harmless in practice: It obviously only
affects callers which pass a NULL domain, and AFAICT the only such
caller is

  async_synchronize_full() ->
  async_synchronize_full_domain(NULL) ->
  async_synchronize_cookie_domain(ASYNC_COOKIE_MAX, NULL)

and the ASYNC_COOKIE_MAX means that in practice we end up waiting for
the async_global_pending list to be empty - but it would break if
somebody happened to pass (void*)-1 as the data element to
async_schedule, and of course also if somebody ever does a
async_synchronize_cookie_domain(, NULL) with a "finite" cookie value.

Cc: sta...@vger.kernel.org # 3.10+
Signed-off-by: Rasmus Villemoes 
---
Maybe the "harmless in practice" means this isn't -stable
material. But I'm not completely confident my quick git grep'ing is
enough, and there might be affected code in one of the earlier kernels
that has since been removed, so I'll leave the decision to the stable
guys.

 kernel/async.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/async.c b/kernel/async.c
index 2cbd3dd5940d..a893d6170944 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -84,20 +84,24 @@ static atomic_t entry_count;
 
 static async_cookie_t lowest_in_progress(struct async_domain *domain)
 {
-   struct list_head *pending;
+   struct async_entry *first = NULL;
async_cookie_t ret = ASYNC_COOKIE_MAX;
unsigned long flags;
 
spin_lock_irqsave(_lock, flags);
 
-   if (domain)
-   pending = >pending;
-   else
-   pending = _global_pending;
+   if (domain) {
+   if (!list_empty(>pending))
+   first = list_first_entry(>pending,
+   struct async_entry, domain_list);
+   } else {
+   if (!list_empty(_global_pending))
+   first = list_first_entry(_global_pending,
+   struct async_entry, global_list);
+   }
 
-   if (!list_empty(pending))
-   ret = list_first_entry(pending, struct async_entry,
-  domain_list)->cookie;
+   if (first)
+   ret = first->cookie;
 
spin_unlock_irqrestore(_lock, flags);
return ret;
-- 
2.11.0

[PATCH] Revert "async: simplify lowest_in_progress()"

[Rasmus Villemoes]

This reverts commit 92266d6ef60c2381c980c6cdcb2a5c1667b36b49, which
was simply wrong: In the case where domain is NULL, we now use the
wrong offsetof() in the list_first_entry macro, so we don't actually
fetch the ->cookie value, but rather the eight bytes located
sizeof(struct list_head) further into the struct async_entry.

On 64 bit, that's the data member, while on 32 bit, we get a u64 built
from func and data in some order.

I think the bug happens to be harmless in practice: It obviously only
affects callers which pass a NULL domain, and AFAICT the only such
caller is

  async_synchronize_full() ->
  async_synchronize_full_domain(NULL) ->
  async_synchronize_cookie_domain(ASYNC_COOKIE_MAX, NULL)

and the ASYNC_COOKIE_MAX means that in practice we end up waiting for
the async_global_pending list to be empty - but it would break if
somebody happened to pass (void*)-1 as the data element to
async_schedule, and of course also if somebody ever does a
async_synchronize_cookie_domain(, NULL) with a "finite" cookie value.

Cc: sta...@vger.kernel.org # 3.10+
Signed-off-by: Rasmus Villemoes 
---
Maybe the "harmless in practice" means this isn't -stable
material. But I'm not completely confident my quick git grep'ing is
enough, and there might be affected code in one of the earlier kernels
that has since been removed, so I'll leave the decision to the stable
guys.

 kernel/async.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/async.c b/kernel/async.c
index 2cbd3dd5940d..a893d6170944 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -84,20 +84,24 @@ static atomic_t entry_count;
 
 static async_cookie_t lowest_in_progress(struct async_domain *domain)
 {
-   struct list_head *pending;
+   struct async_entry *first = NULL;
async_cookie_t ret = ASYNC_COOKIE_MAX;
unsigned long flags;
 
spin_lock_irqsave(_lock, flags);
 
-   if (domain)
-   pending = >pending;
-   else
-   pending = _global_pending;
+   if (domain) {
+   if (!list_empty(>pending))
+   first = list_first_entry(>pending,
+   struct async_entry, domain_list);
+   } else {
+   if (!list_empty(_global_pending))
+   first = list_first_entry(_global_pending,
+   struct async_entry, global_list);
+   }
 
-   if (!list_empty(pending))
-   ret = list_first_entry(pending, struct async_entry,
-  domain_list)->cookie;
+   if (first)
+   ret = first->cookie;
 
spin_unlock_irqrestore(_lock, flags);
return ret;
-- 
2.11.0