Discussion:
4088 use after free in arc_release(), 4089 NULL pointer dereference in arc_read()
Boris Protopopov
2013-09-03 15:59:45 UTC
Permalink
Hi, guys,

I have quick fix for the use after free and the NULL pointer dereference,
the bugs are documented at https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.

Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/

In both cases, it is fairly clear how the bugs are addressed; we have been
running with the fix for 4088 for a while, the 4089 is under testing at
this time.
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Garrett D'Amore
2013-09-03 17:32:32 UTC
Permalink
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference, the bugs are documented at https://www.illumos.org/issues/4088 and https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have been running with the fix for 4088 for a while, the 4089 is under testing at this time.
4088 looks ok, though I'm not an expert in the ARC use of lists.

4089 looks fishy though. In particular, before the changes to avoid the use of hdr->b_l2hdr, there is this check:

3072 if (hdr->b_l2hdr != NULL &&
3073 !HDR_L2_WRITING(hdr) && !HDR_L2_EVICTED(hdr) &&
3074 !(l2arc_noprefetch && HDR_PREFETCH(hdr))) {
3075 l2arc_read_callback_t *cb;

If you're hitting problems, then I suppose you must not be holding a lock that you should, as the hdr->b_l2hdr should point to valid data. Am I misunderstanding or missing something here? (Perhaps the lock that needs to be held is the hash lock, and the value of b_compress should be collected before the lock is dropped? I'm not sure. It just feels fishy to me.

- Garrett





-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-03 17:38:34 UTC
Permalink
The problem in 4089 is that the b_l2hdr can be set to zero, so if we are outside the locked section, we should not check/dereference. Other fields are set atomically (properly aligned), so one can read them without locks.

Additionally, the L2ARC read code knows how to deal with evictions at any point, so it would seem that this is the minimal change needed.

Typos courtesy of my iPhone
Post by Garrett D'Amore
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference, the bugs are documented at https://www.illumos.org/issues/4088 and https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have been running with the fix for 4088 for a while, the 4089 is under testing at this time.
4088 looks ok, though I'm not an expert in the ARC use of lists.
3072 if (hdr->b_l2hdr != NULL &&
3073 !HDR_L2_WRITING(hdr) && !HDR_L2_EVICTED(hdr) &&
3074 !(l2arc_noprefetch && HDR_PREFETCH(hdr))) {
3075 l2arc_read_callback_t *cb;
If you're hitting problems, then I suppose you must not be holding a lock that you should, as the hdr->b_l2hdr should point to valid data. Am I misunderstanding or missing something here? (Perhaps the lock that needs to be held is the hash lock, and the value of b_compress should be collected before the lock is dropped? I'm not sure. It just feels fishy to me.
- Garrett
illumos-zfs | Archives | Modify Your Subscription
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-03 17:45:38 UTC
Permalink
Sorry, Garret,
it's a mistake on my part, the intent was in fact to bet b_compress under lock.

Typos courtesy of my iPhone
Post by Garrett D'Amore
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference, the bugs are documented at https://www.illumos.org/issues/4088 and https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have been running with the fix for 4088 for a while, the 4089 is under testing at this time.
4088 looks ok, though I'm not an expert in the ARC use of lists.
3072 if (hdr->b_l2hdr != NULL &&
3073 !HDR_L2_WRITING(hdr) && !HDR_L2_EVICTED(hdr) &&
3074 !(l2arc_noprefetch && HDR_PREFETCH(hdr))) {
3075 l2arc_read_callback_t *cb;
If you're hitting problems, then I suppose you must not be holding a lock that you should, as the hdr->b_l2hdr should point to valid data. Am I misunderstanding or missing something here? (Perhaps the lock that needs to be held is the hash lock, and the value of b_compress should be collected before the lock is dropped? I'm not sure. It just feels fishy to me.
- Garrett
illumos-zfs | Archives | Modify Your Subscription
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-03 18:47:47 UTC
Permalink
Yes, some ill-advised last moment changes introduced problems pointed out
by Garrett.

To clarify, the original issue to be addressed was as follows: there was a
race between checking if b_l2hdr was NULL and subsequent de-reference of
that pointer (when it could already be NULL).

The approach chosen to fix this was to stash the values of the b_size and
b_compress fields used outside the locked section. That eliminated the need
to de-reference. The remaining checks done outside the locked section did
not seem to require the lock held.

The webrev for 4089 updated in place:

b_compress is assigned under lock if b_l2hdr is not NULL, and is used later
only if b_l2hdr is not NULL.

Boris.


On Tue, Sep 3, 2013 at 1:45 PM, Boris Protopopov <
Post by Boris Protopopov
Sorry, Garret,
it's a mistake on my part, the intent was in fact to bet b_compress under lock.
Typos courtesy of my iPhone
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference,
the bugs are documented at https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have been
running with the fix for 4088 for a while, the 4089 is under testing at
this time.
4088 looks ok, though I'm not an expert in the ARC use of lists.
4089 looks fishy though. In particular, before the changes to avoid the
3072 if (hdr->b_l2hdr != NULL &&
3073 !HDR_L2_WRITING(hdr) && !HDR_L2_EVICTED(hdr) &&
3074 !(l2arc_noprefetch && HDR_PREFETCH(hdr))) {
3075 l2arc_read_callback_t *cb;
If you're hitting problems, then I suppose you must not be holding a lock
that you should, as the hdr->b_l2hdr should point to valid data. Am I
misunderstanding or missing something here? (Perhaps the lock that needs
to be held is the hash lock, and the value of b_compress should be
collected before the lock is dropped? I'm not sure. It just feels fishy
to me.
- Garrett
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Garrett D'Amore
2013-09-03 19:26:15 UTC
Permalink
Yes, some ill-advised last moment changes introduced problems pointed out by Garrett.
To clarify, the original issue to be addressed was as follows: there was a race between checking if b_l2hdr was NULL and subsequent de-reference of that pointer (when it could already be NULL).
The approach chosen to fix this was to stash the values of the b_size and b_compress fields used outside the locked section. That eliminated the need to de-reference. The remaining checks done outside the locked section did not seem to require the lock held.
b_compress is assigned under lock if b_l2hdr is not NULL, and is used later only if b_l2hdr is not NULL.
So what about size, is hdr->b_l2hdr->b_asize
always guarantted to be the same as "size" (size is assigned by BP_GET_LSIZE(bp) higher up in the function). Can it change while the lock is dropped?

Why are we dropping the hash lock as early as we are? I presume this is a contended lock, and the idea is not to hold it across the call to zio_read()? It seems like what's needed here is a two-level lock scheme -- one to hold the buffer and header against changes, and another to hold the hash. Admittedly, I've not followed the threads about rearchitecting the ARC locks very closely, but I'd like to have someone who's more familiar with the locking internals of ARC review this code closely.

- Garrett
Boris.
Sorry, Garret,
it's a mistake on my part, the intent was in fact to bet b_compress under lock.
Typos courtesy of my iPhone
Post by Garrett D'Amore
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference, the bugs are documented at https://www.illumos.org/issues/4088 and https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have been running with the fix for 4088 for a while, the 4089 is under testing at this time.
4088 looks ok, though I'm not an expert in the ARC use of lists.
3072 if (hdr->b_l2hdr != NULL &&
3073 !HDR_L2_WRITING(hdr) && !HDR_L2_EVICTED(hdr) &&
3074 !(l2arc_noprefetch && HDR_PREFETCH(hdr))) {
3075 l2arc_read_callback_t *cb;
If you're hitting problems, then I suppose you must not be holding a lock that you should, as the hdr->b_l2hdr should point to valid data. Am I misunderstanding or missing something here? (Perhaps the lock that needs to be held is the hash lock, and the value of b_compress should be collected before the lock is dropped? I'm not sure. It just feels fishy to me.
- Garrett
illumos-zfs | Archives | Modify Your Subscription
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
illumos-zfs | Archives | Modify Your Subscription
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-03 20:13:50 UTC
Permalink
I do not believe the size can change. This is a size of the read, and
presumably, it is valid :) for the duration of the read, and we are about
to issue that read at the code point in question.

To provide more context, the NULL pointer deref was inadvertently
introduced recently, and the size has been stashed away previously, and
"worked", so this is not something new that is introduced by this change
but rather a "revert to old working code".

Yes, the idea is to drop the hash lock before getting into zio processing,
because as Brendan suggests (above), there is potential for deadlocks.
Also, I do not like the idea of holding locks and going to sleep (in memory
allocations, for instance).
On Sep 3, 2013, at 11:47 AM, Boris Protopopov <
Yes, some ill-advised last moment changes introduced problems pointed out by Garrett.
To clarify, the original issue to be addressed was as follows: there was a
race between checking if b_l2hdr was NULL and subsequent de-reference of
that pointer (when it could already be NULL).
The approach chosen to fix this was to stash the values of the b_size and
b_compress fields used outside the locked section. That eliminated the need
to de-reference. The remaining checks done outside the locked section did
not seem to require the lock held.
b_compress is assigned under lock if b_l2hdr is not NULL, and is used
later only if b_l2hdr is not NULL.
So what about size, is hdr->b_l2hdr->b_asize
always guarantted to be the same as "size" (size is assigned
by BP_GET_LSIZE(bp) higher up in the function). Can it change while the
lock is dropped?
Why are we dropping the hash lock as early as we are? I presume this is a
contended lock, and the idea is not to hold it across the call to
zio_read()? It seems like what's needed here is a two-level lock scheme --
one to hold the buffer and header against changes, and another to hold the
hash. Admittedly, I've not followed the threads about rearchitecting the
ARC locks very closely, but I'd like to have someone who's more familiar
with the locking internals of ARC review this code closely.
- Garrett
Boris.
On Tue, Sep 3, 2013 at 1:45 PM, Boris Protopopov <
Post by Boris Protopopov
Sorry, Garret,
it's a mistake on my part, the intent was in fact to bet b_compress under lock.
Typos courtesy of my iPhone
On Sep 3, 2013, at 8:59 AM, Boris Protopopov <
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference,
the bugs are documented at https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under testing
at this time.
4088 looks ok, though I'm not an expert in the ARC use of lists.
4089 looks fishy though. In particular, before the changes to avoid the
3072 if (hdr->b_l2hdr != NULL &&
3073 !HDR_L2_WRITING(hdr) && !HDR_L2_EVICTED(hdr) &&
3074 !(l2arc_noprefetch && HDR_PREFETCH(hdr))) {
3075 l2arc_read_callback_t *cb;
If you're hitting problems, then I suppose you must not be holding a lock
that you should, as the hdr->b_l2hdr should point to valid data. Am I
misunderstanding or missing something here? (Perhaps the lock that needs
to be held is the hash lock, and the value of b_compress should be
collected before the lock is dropped? I'm not sure. It just feels fishy
to me.
- Garrett
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com/>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/22035932-85c5d227> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com/>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-03 20:38:18 UTC
Permalink
While on the topic of the sizes,

it appears that b_size and b_l2hdr->b_asize could be different, so, I
should be stashing the latter instead of reusing the b_size, which I was
doing earlier anyway. The webrev is updated in place.

Boris.



On Tue, Sep 3, 2013 at 4:13 PM, Boris Protopopov <
Post by Boris Protopopov
I do not believe the size can change. This is a size of the read, and
presumably, it is valid :) for the duration of the read, and we are about
to issue that read at the code point in question.
To provide more context, the NULL pointer deref was inadvertently
introduced recently, and the size has been stashed away previously, and
"worked", so this is not something new that is introduced by this change
but rather a "revert to old working code".
Yes, the idea is to drop the hash lock before getting into zio processing,
because as Brendan suggests (above), there is potential for deadlocks.
Also, I do not like the idea of holding locks and going to sleep (in memory
allocations, for instance).
On Sep 3, 2013, at 11:47 AM, Boris Protopopov <
Yes, some ill-advised last moment changes introduced problems pointed out by Garrett.
To clarify, the original issue to be addressed was as follows: there was
a race between checking if b_l2hdr was NULL and subsequent de-reference of
that pointer (when it could already be NULL).
The approach chosen to fix this was to stash the values of the b_size and
b_compress fields used outside the locked section. That eliminated the need
to de-reference. The remaining checks done outside the locked section did
not seem to require the lock held.
b_compress is assigned under lock if b_l2hdr is not NULL, and is used
later only if b_l2hdr is not NULL.
So what about size, is hdr->b_l2hdr->b_asize
always guarantted to be the same as "size" (size is assigned
by BP_GET_LSIZE(bp) higher up in the function). Can it change while the
lock is dropped?
Why are we dropping the hash lock as early as we are? I presume this is
a contended lock, and the idea is not to hold it across the call to
zio_read()? It seems like what's needed here is a two-level lock scheme --
one to hold the buffer and header against changes, and another to hold the
hash. Admittedly, I've not followed the threads about rearchitecting the
ARC locks very closely, but I'd like to have someone who's more familiar
with the locking internals of ARC review this code closely.
- Garrett
Boris.
On Tue, Sep 3, 2013 at 1:45 PM, Boris Protopopov <
Post by Boris Protopopov
Sorry, Garret,
it's a mistake on my part, the intent was in fact to bet b_compress under lock.
Typos courtesy of my iPhone
On Sep 3, 2013, at 8:59 AM, Boris Protopopov <
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under testing
at this time.
4088 looks ok, though I'm not an expert in the ARC use of lists.
4089 looks fishy though. In particular, before the changes to avoid the
3072 if (hdr->b_l2hdr != NULL &&
3073 !HDR_L2_WRITING(hdr) && !HDR_L2_EVICTED(hdr) &&
3074 !(l2arc_noprefetch && HDR_PREFETCH(hdr))) {
3075 l2arc_read_callback_t *cb;
If you're hitting problems, then I suppose you must not be holding a
lock that you should, as the hdr->b_l2hdr should point to valid data. Am I
misunderstanding or missing something here? (Perhaps the lock that needs
to be held is the hash lock, and the value of b_compress should be
collected before the lock is dropped? I'm not sure. It just feels fishy
to me.
- Garrett
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com/>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/22035932-85c5d227> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com/>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-09-04 12:12:33 UTC
Permalink
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under
testing at this time.
LGTM.
--
Saso
Matthew Ahrens
2013-09-08 12:36:17 UTC
Permalink
4088: why not save the b_compress / b_asize in the same block that we're
already saving b_daddr, just above your changes?

--matt


On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference,
the bugs are documented at https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have been
running with the fix for 4088 for a while, the 4089 is under testing at
this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-09 14:07:01 UTC
Permalink
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.

I can think of a situation, when someone changes secondarycache property to
"none", for instance, yet the data is still in L2ARC, so it will be read,
yet we will not attempt to cache it in L2ARC, and we will still get the
same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that we're
already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference,
the bugs are documented at https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under testing
at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Matthew Ahrens
2013-09-12 23:35:49 UTC
Permalink
Wouldn't that argument also apply to "addr"? It's being used in the same
statement where you're using "b_asize". Why it is OK to set "addr" from
within the "if (HDR_L2CACHE" block, but not to set "b_asize"?

--matt


On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache property
to "none", for instance, yet the data is still in L2ARC, so it will be
read, yet we will not attempt to cache it in L2ARC, and we will still get
the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that we're
already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under testing
at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-12 23:53:32 UTC
Permalink
Matt,

actually, after looking at this a bit more, I believe we can indeed move
setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause. The
reason is that the devw is set to B_TRUE and then checked below, so we will
only use the addr, b_asize, b_compress below only if HDR_L2CACHE() holds.

This said, I do believe that whether or not we are going to cache this data
is a separate question from whether or not it is already in L2ARC. It
appears that the present code makes a simplifying assumption - if we are
not going to cache, we will not read from l2arc either. It is likely that
in general, this is an OK simplification. However, it seems that it is a
unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).

Do you think this is something to be fixed ?

Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in the same
statement where you're using "b_asize". Why it is OK to set "addr" from
within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache property
to "none", for instance, yet the data is still in L2ARC, so it will be
read, yet we will not attempt to cache it in L2ARC, and we will still get
the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that we're
already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under testing
at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Matthew Ahrens
2013-09-13 00:12:20 UTC
Permalink
On Fri, Sep 13, 2013 at 2:53 AM, Boris Protopopov <
Post by Boris Protopopov
Matt,
actually, after looking at this a bit more, I believe we can indeed move
setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause. The
reason is that the devw is set to B_TRUE and then checked below, so we will
only use the addr, b_asize, b_compress below only if HDR_L2CACHE() holds.
I don't see that. We set devw to l2ad_writing, which might be true or
false, and we can still get to the zio_read_phys() call if devw==B_FALSE
(the default).
Post by Boris Protopopov
This said, I do believe that whether or not we are going to cache this
data is a separate question from whether or not it is already in L2ARC. It
appears that the present code makes a simplifying assumption - if we are
not going to cache, we will not read from l2arc either. It is likely that
in general, this is an OK simplification. However, it seems that it is a
unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).
That analysis makes sense to me.
Post by Boris Protopopov
Do you think this is something to be fixed ?
Sure. So we don't check HDR_L2CACHE() here, and we save all the variables
inside the existing block. Makes sense to me:

if (hdr->b_l2hdr != NULL && (vd = ...)) {
devw = ...;
b_addr = ...;
b_compress = ...;
b_asize = ...;
...
}

--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in the same
statement where you're using "b_asize". Why it is OK to set "addr" from
within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache property
to "none", for instance, yet the data is still in L2ARC, so it will be
read, yet we will not attempt to cache it in L2ARC, and we will still get
the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that
we're already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at
http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under testing
at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-13 00:18:54 UTC
Permalink
Yes, I misspoke, devw could indeed be false after visiting the if
(HDR_L2CACHE()...) {} clause.

What I should have said, was that devw could only be true if we visited the
if (HDR_L2CACHE()...) {} clause. Since we only use what we set there if
devw is true, it is safe to set b_asize and b_compress there.

Boris.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 2:53 AM, Boris Protopopov <
Post by Boris Protopopov
Matt,
actually, after looking at this a bit more, I believe we can indeed move
setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause. The
reason is that the devw is set to B_TRUE and then checked below, so we will
only use the addr, b_asize, b_compress below only if HDR_L2CACHE() holds.
I don't see that. We set devw to l2ad_writing, which might be true or
false, and we can still get to the zio_read_phys() call if devw==B_FALSE
(the default).
Post by Boris Protopopov
This said, I do believe that whether or not we are going to cache this
data is a separate question from whether or not it is already in L2ARC. It
appears that the present code makes a simplifying assumption - if we are
not going to cache, we will not read from l2arc either. It is likely that
in general, this is an OK simplification. However, it seems that it is a
unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).
That analysis makes sense to me.
Post by Boris Protopopov
Do you think this is something to be fixed ?
Sure. So we don't check HDR_L2CACHE() here, and we save all the variables
if (hdr->b_l2hdr != NULL && (vd = ...)) {
devw = ...;
b_addr = ...;
b_compress = ...;
b_asize = ...;
...
}
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in the
same statement where you're using "b_asize". Why it is OK to set "addr"
from within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache
property to "none", for instance, yet the data is still in L2ARC, so it
will be read, yet we will not attempt to cache it in L2ARC, and we will
still get the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that
we're already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at
http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been running with the fix for 4088 for a while, the 4089 is under testing
at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Matthew Ahrens
2013-09-13 00:28:56 UTC
Permalink
On Fri, Sep 13, 2013 at 3:18 AM, Boris Protopopov <
Post by Boris Protopopov
Yes, I misspoke, devw could indeed be false after visiting the if
(HDR_L2CACHE()...) {} clause.
What I should have said, was that devw could only be true if we visited
the if (HDR_L2CACHE()...) {} clause.
Agreed.
Post by Boris Protopopov
Since we only use what we set there if devw is true, it is safe to set
b_asize and b_compress there.
I don't think that is the case. As I said, we can still get to the
zio_read_phys() and use "addr" even if devw is FALSE. The only check is:

if (vd != NULL && l2arc_ndev != 0 && !(l2arc_norw && devw)) {
which can test true if devw==false.

However, it looks like the "vd != NULL" check here will save us, because if
"vd" is set, then must have executed the block that sets "addr". I missed
this on my first reading (got to love side effects in "if" statements, ugh).

--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 2:53 AM, Boris Protopopov <
Post by Boris Protopopov
Matt,
actually, after looking at this a bit more, I believe we can indeed move
setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause. The
reason is that the devw is set to B_TRUE and then checked below, so we will
only use the addr, b_asize, b_compress below only if HDR_L2CACHE() holds.
I don't see that. We set devw to l2ad_writing, which might be true or
false, and we can still get to the zio_read_phys() call if devw==B_FALSE
(the default).
Post by Boris Protopopov
This said, I do believe that whether or not we are going to cache this
data is a separate question from whether or not it is already in L2ARC. It
appears that the present code makes a simplifying assumption - if we are
not going to cache, we will not read from l2arc either. It is likely that
in general, this is an OK simplification. However, it seems that it is a
unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).
That analysis makes sense to me.
Post by Boris Protopopov
Do you think this is something to be fixed ?
Sure. So we don't check HDR_L2CACHE() here, and we save all the
if (hdr->b_l2hdr != NULL && (vd = ...)) {
devw = ...;
b_addr = ...;
b_compress = ...;
b_asize = ...;
...
}
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in the
same statement where you're using "b_asize". Why it is OK to set "addr"
from within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache
property to "none", for instance, yet the data is still in L2ARC, so it
will be read, yet we will not attempt to cache it in L2ARC, and we will
still get the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that
we're already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at
http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we
have been running with the fix for 4088 for a while, the 4089 is under
testing at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-13 00:33:20 UTC
Permalink
Yes, same here, I concur. I will update the webrev.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 3:18 AM, Boris Protopopov <
Post by Boris Protopopov
Yes, I misspoke, devw could indeed be false after visiting the if
(HDR_L2CACHE()...) {} clause.
What I should have said, was that devw could only be true if we visited
the if (HDR_L2CACHE()...) {} clause.
Agreed.
Post by Boris Protopopov
Since we only use what we set there if devw is true, it is safe to set
b_asize and b_compress there.
I don't think that is the case. As I said, we can still get to the
if (vd != NULL && l2arc_ndev != 0 && !(l2arc_norw && devw)) {
which can test true if devw==false.
However, it looks like the "vd != NULL" check here will save us, because
if "vd" is set, then must have executed the block that sets "addr". I
missed this on my first reading (got to love side effects in "if"
statements, ugh).
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 2:53 AM, Boris Protopopov <
Post by Boris Protopopov
Matt,
actually, after looking at this a bit more, I believe we can indeed
move setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause.
The reason is that the devw is set to B_TRUE and then checked below, so we
will only use the addr, b_asize, b_compress below only if HDR_L2CACHE()
holds.
I don't see that. We set devw to l2ad_writing, which might be true or
false, and we can still get to the zio_read_phys() call if devw==B_FALSE
(the default).
Post by Boris Protopopov
This said, I do believe that whether or not we are going to cache this
data is a separate question from whether or not it is already in L2ARC. It
appears that the present code makes a simplifying assumption - if we are
not going to cache, we will not read from l2arc either. It is likely that
in general, this is an OK simplification. However, it seems that it is a
unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).
That analysis makes sense to me.
Post by Boris Protopopov
Do you think this is something to be fixed ?
Sure. So we don't check HDR_L2CACHE() here, and we save all the
if (hdr->b_l2hdr != NULL && (vd = ...)) {
devw = ...;
b_addr = ...;
b_compress = ...;
b_asize = ...;
...
}
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in the
same statement where you're using "b_asize". Why it is OK to set "addr"
from within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache
property to "none", for instance, yet the data is still in L2ARC, so it
will be read, yet we will not attempt to cache it in L2ARC, and we will
still get the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that
we're already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at
http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we
have been running with the fix for 4088 for a while, the 4089 is under
testing at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-09-13 00:58:48 UTC
Permalink
webrev updated


On Thu, Sep 12, 2013 at 8:33 PM, Boris Protopopov <
Post by Boris Protopopov
Yes, same here, I concur. I will update the webrev.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 3:18 AM, Boris Protopopov <
Post by Boris Protopopov
Yes, I misspoke, devw could indeed be false after visiting the if
(HDR_L2CACHE()...) {} clause.
What I should have said, was that devw could only be true if we visited
the if (HDR_L2CACHE()...) {} clause.
Agreed.
Post by Boris Protopopov
Since we only use what we set there if devw is true, it is safe to set
b_asize and b_compress there.
I don't think that is the case. As I said, we can still get to the
if (vd != NULL && l2arc_ndev != 0 && !(l2arc_norw && devw)) {
which can test true if devw==false.
However, it looks like the "vd != NULL" check here will save us, because
if "vd" is set, then must have executed the block that sets "addr". I
missed this on my first reading (got to love side effects in "if"
statements, ugh).
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 2:53 AM, Boris Protopopov <
Post by Boris Protopopov
Matt,
actually, after looking at this a bit more, I believe we can indeed
move setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause.
The reason is that the devw is set to B_TRUE and then checked below, so we
will only use the addr, b_asize, b_compress below only if HDR_L2CACHE()
holds.
I don't see that. We set devw to l2ad_writing, which might be true or
false, and we can still get to the zio_read_phys() call if devw==B_FALSE
(the default).
Post by Boris Protopopov
This said, I do believe that whether or not we are going to cache this
data is a separate question from whether or not it is already in L2ARC. It
appears that the present code makes a simplifying assumption - if we are
not going to cache, we will not read from l2arc either. It is likely that
in general, this is an OK simplification. However, it seems that it is a
unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).
That analysis makes sense to me.
Post by Boris Protopopov
Do you think this is something to be fixed ?
Sure. So we don't check HDR_L2CACHE() here, and we save all the
if (hdr->b_l2hdr != NULL && (vd = ...)) {
devw = ...;
b_addr = ...;
b_compress = ...;
b_asize = ...;
...
}
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in the
same statement where you're using "b_asize". Why it is OK to set "addr"
from within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache
property to "none", for instance, yet the data is still in L2ARC, so it
will be read, yet we will not attempt to cache it in L2ARC, and we will
still get the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that
we're already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at
http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we
have been running with the fix for 4088 for a while, the 4089 is under
testing at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Matthew Ahrens
2013-09-15 02:06:36 UTC
Permalink
Both changes look good. Please send an RTI email at your convenience.

http://wiki.illumos.org/display/illumos/How+To+Contribute#HowToContribute-5SubmittingAPatch

--matt


On Thu, Sep 12, 2013 at 5:58 PM, Boris Protopopov <
Post by Boris Protopopov
webrev updated
On Thu, Sep 12, 2013 at 8:33 PM, Boris Protopopov <
Post by Boris Protopopov
Yes, same here, I concur. I will update the webrev.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 3:18 AM, Boris Protopopov <
Post by Boris Protopopov
Yes, I misspoke, devw could indeed be false after visiting the if
(HDR_L2CACHE()...) {} clause.
What I should have said, was that devw could only be true if we visited
the if (HDR_L2CACHE()...) {} clause.
Agreed.
Post by Boris Protopopov
Since we only use what we set there if devw is true, it is safe to set
b_asize and b_compress there.
I don't think that is the case. As I said, we can still get to the
if (vd != NULL && l2arc_ndev != 0 && !(l2arc_norw && devw)) {
which can test true if devw==false.
However, it looks like the "vd != NULL" check here will save us, because
if "vd" is set, then must have executed the block that sets "addr". I
missed this on my first reading (got to love side effects in "if"
statements, ugh).
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 2:53 AM, Boris Protopopov <
Post by Boris Protopopov
Matt,
actually, after looking at this a bit more, I believe we can indeed
move setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause.
The reason is that the devw is set to B_TRUE and then checked below, so we
will only use the addr, b_asize, b_compress below only if HDR_L2CACHE()
holds.
I don't see that. We set devw to l2ad_writing, which might be true or
false, and we can still get to the zio_read_phys() call if devw==B_FALSE
(the default).
Post by Boris Protopopov
This said, I do believe that whether or not we are going to cache
this data is a separate question from whether or not it is already in
L2ARC. It appears that the present code makes a simplifying assumption - if
we are not going to cache, we will not read from l2arc either. It is likely
that in general, this is an OK simplification. However, it seems that it is
a unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).
That analysis makes sense to me.
Post by Boris Protopopov
Do you think this is something to be fixed ?
Sure. So we don't check HDR_L2CACHE() here, and we save all the
if (hdr->b_l2hdr != NULL && (vd = ...)) {
devw = ...;
b_addr = ...;
b_compress = ...;
b_asize = ...;
...
}
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in the
same statement where you're using "b_asize". Why it is OK to set "addr"
from within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks for
HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache
property to "none", for instance, yet the data is still in L2ARC, so it
will be read, yet we will not attempt to cache it in L2ARC, and we will
still get the same crash, as it seems.
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block that
we're already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at
http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we
have been running with the fix for 4088 for a while, the 4089 is under
testing at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc> |
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Matthew Ahrens
2013-10-18 18:06:08 UTC
Permalink
It's been a month and I don't see these changes in Illumos. Did you send
an RTI?

--matt
Post by Matthew Ahrens
Both changes look good. Please send an RTI email at your convenience.
http://wiki.illumos.org/display/illumos/How+To+Contribute#HowToContribute-5SubmittingAPatch
--matt
On Thu, Sep 12, 2013 at 5:58 PM, Boris Protopopov <
Post by Boris Protopopov
webrev updated
On Thu, Sep 12, 2013 at 8:33 PM, Boris Protopopov <
Post by Boris Protopopov
Yes, same here, I concur. I will update the webrev.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 3:18 AM, Boris Protopopov <
Post by Boris Protopopov
Yes, I misspoke, devw could indeed be false after visiting the if
(HDR_L2CACHE()...) {} clause.
What I should have said, was that devw could only be true if we
visited the if (HDR_L2CACHE()...) {} clause.
Agreed.
Post by Boris Protopopov
Since we only use what we set there if devw is true, it is safe to set
b_asize and b_compress there.
I don't think that is the case. As I said, we can still get to the
if (vd != NULL && l2arc_ndev != 0 && !(l2arc_norw && devw)) {
which can test true if devw==false.
However, it looks like the "vd != NULL" check here will save us,
because if "vd" is set, then must have executed the block that sets "addr".
I missed this on my first reading (got to love side effects in "if"
statements, ugh).
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
On Fri, Sep 13, 2013 at 2:53 AM, Boris Protopopov <
Post by Boris Protopopov
Matt,
actually, after looking at this a bit more, I believe we can indeed
move setting b_asize and b_compress in the if (HDR_L2CACHE()...) clause.
The reason is that the devw is set to B_TRUE and then checked below, so we
will only use the addr, b_asize, b_compress below only if HDR_L2CACHE()
holds.
I don't see that. We set devw to l2ad_writing, which might be true
or false, and we can still get to the zio_read_phys() call if devw==B_FALSE
(the default).
Post by Boris Protopopov
This said, I do believe that whether or not we are going to cache
this data is a separate question from whether or not it is already in
L2ARC. It appears that the present code makes a simplifying assumption - if
we are not going to cache, we will not read from l2arc either. It is likely
that in general, this is an OK simplification. However, it seems that it is
a unnecessary simplification because we know with 100% certainty which data
is and which is not in l2arc (b_l2hdr).
That analysis makes sense to me.
Post by Boris Protopopov
Do you think this is something to be fixed ?
Sure. So we don't check HDR_L2CACHE() here, and we save all the
if (hdr->b_l2hdr != NULL && (vd = ...)) {
devw = ...;
b_addr = ...;
b_compress = ...;
b_asize = ...;
...
}
--matt
Post by Boris Protopopov
Boris.
Post by Matthew Ahrens
Wouldn't that argument also apply to "addr"? It's being used in
the same statement where you're using "b_asize". Why it is OK to set
"addr" from within the "if (HDR_L2CACHE" block, but not to set "b_asize"?
--matt
On Mon, Sep 9, 2013 at 5:07 PM, Boris Protopopov <
Post by Boris Protopopov
I took a look at it;it appears that the if() in question checks
for HDR_L2CACHE(), which, as I understand it, means "after we read this, we
will allow this to be cached in L2 ARC". Yet the region we are trying to
"protect" from dereferencing the b_l2hdr, does not perform the L2
cacheability check, which means we can potentially visit that region
without visiting the first if() clause, as I understand it.
I can think of a situation, when someone changes secondarycache
property to "none", for instance, yet the data is still in L2ARC, so it
will be read, yet we will not attempt to cache it in L2ARC, and we will
still get the same crash, as it seems.
On Sun, Sep 8, 2013 at 8:36 AM, Matthew Ahrens <
Post by Matthew Ahrens
4088: why not save the b_compress / b_asize in the same block
that we're already saving b_daddr, just above your changes?
--matt
On Tue, Sep 3, 2013 at 6:59 PM, Boris Protopopov <
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer
dereference, the bugs are documented at
https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at
http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we
have been running with the fix for 4088 for a while, the 4089 is under
testing at this time.
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/23052084-8e2408bc>|
Modify <https://www.listbox.com/member/?&> Your Subscription
<http://www.listbox.com>
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
--
Best regards,
Boris Protopopov
Nexenta Systems
455 El Camino Real, Santa Clara, CA 95050
[d] 408.791.3366 | [c] 978.621.6901 Skype: bprotopopov
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> |
Modify<https://www.listbox.com/member/?&>Your Subscription
<http://www.listbox.com>
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Richard Yao
2013-10-19 00:04:34 UTC
Permalink
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference,
the bugs are documented at https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/ and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have been
running with the fix for 4088 for a while, the 4089 is under testing at
this time.
This might solve the 'buffer modified while frozen!' issue mentioned in
3113 (the same one currently blocking inclusion of nop write on ZoL):

https://www.illumos.org/issues/3113

Would someone who can reliably reproduce this check to see if that is
the case?




-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Boris Protopopov
2013-10-19 01:03:34 UTC
Permalink
The 4088 was only caught once under significant load by way of COMSTAR. We
caught it with kmem_flags set, which allowed it to be diagnosed (details in
the issue description). As "use after free" goes, the side effects are
ill-defined and often delayed, so hard to say how frequently this was
observed.

If we want to see whether the "modified when frozen" is the same or
similar, I recommend using debug aids analogous to kmem_flags with ztest's
umem to catch use after free (including the use of freed then re-allocated
buffers).

The 4089 as something repro-ed by Andriy, not really sure how often that
arises.

Boris.
Post by Richard Yao
Post by Boris Protopopov
Hi, guys,
I have quick fix for the use after free and the NULL pointer dereference,
the bugs are documented at https://www.illumos.org/issues/4088 and
https://www.illumos.org/issues/4089.
Please take a look at the webrevs at http://ma.nexenta.com/borisp/4088/and
http://ma.nexenta.com/borisp/4089/
In both cases, it is fairly clear how the bugs are addressed; we have
been
Post by Boris Protopopov
running with the fix for 4088 for a while, the 4089 is under testing at
this time.
This might solve the 'buffer modified while frozen!' issue mentioned in
https://www.illumos.org/issues/3113
Would someone who can reliably reproduce this check to see if that is
the case?
--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Loading...