ceph: Fix error handling in ceph_read_iter

PATCH Superseded Nikolay Borisov 3 participants 7 messages Oct 10, 2016 Ceph client

Share & tools

Nikolay Borisov

author

2016-10-10 12:38 UTC #0 lore raw

PATCH 1/1 ceph: Fix error handling in ceph_read_iter

In case __ceph_do_getattr returns an error and the retry_op in ceph_read_iter is not READ_INLINE, then it's possible to invoke __free_page on a page which is NULL, this naturally leads to a crash. This can happen when, for example, a process waiting on a MDS reply receives sigterm.

Fix this by explicitly checking whether the page is set or not.

Signed-off-by Nikolay Borisov <[email protected]>

Link http://www.spinics.net/lists/ceph-users/msg31592.html

---
 fs/ceph/file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/ceph/file.c +2 -1

index 3c68e6aee2f0..7413313ae6c8 100644
@@ -929,7 +929,8 @@ again:
 		statret = __ceph_do_getattr(inode, page,
 					    CEPH_STAT_CAP_INLINE_DATA, !!page);
 		if (statret < 0) {
-			 __free_page(page);
+			if (!page)
+				__free_page(page);
 			if (statret == -ENODATA) {
 				BUG_ON(retry_op != READ_INLINE);
 				goto again;

Signature · 1 line

2.5.0

Nikolay Borisov

author

2016-10-10 12:56 UTC #1 lore raw

PATCH v2 1/1 ceph: Fix error handling in ceph_read_iter

Fix this by explicitly checking whether the page is set or not.

Signed-off-by Nikolay Borisov <[email protected]>

Link http://www.spinics.net/lists/ceph-users/msg31592.html

---

Inverted the condition, so resending with correct condition this time.

 fs/ceph/file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/ceph/file.c +2 -1

index 3c68e6aee2f0..7413313ae6c8 100644
@@ -929,7 +929,8 @@ again:
 		statret = __ceph_do_getattr(inode, page,
 					    CEPH_STAT_CAP_INLINE_DATA, !!page);
 		if (statret < 0) {
-			 __free_page(page);
+			if (page)
+				__free_page(page);
 			if (statret == -ENODATA) {
 				BUG_ON(retry_op != READ_INLINE);
 				goto again;

Signature · 1 line

2.5.0

Yan, Zheng

Reviewed-by on v2 1/1

2016-10-10 13:11 UTC #2 lore raw

Quoted 35 lines · Nikolay Borisov

In case __ceph_do_getattr returns an error and the retry_op in ceph_read_iter is not READ_INLINE, then it's possible to invoke __free_page on a page which is NULL, this naturally leads to a crash. This can happen when, for example, a process waiting on a MDS reply receives sigterm.
Fix this by explicitly checking whether the page is set or not.
Signed-off-by Nikolay Borisov <[email protected]>
Link http://www.spinics.net/lists/ceph-users/msg31592.html
---
Inverted the condition, so resending with correct condition this time.
fs/ceph/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
fs/ceph/file.c +2 -1
index 3c68e6aee2f0..7413313ae6c8 100644
@@ -929,7 +929,8 @@ again:
 		statret = __ceph_do_getattr(inode, page,
 					    CEPH_STAT_CAP_INLINE_DATA, !!page);
 		if (statret < 0) {
-			 __free_page(page);
+			if (page)
+				__free_page(page);
 			if (statret == -ENODATA) {
 				BUG_ON(retry_op != READ_INLINE);
 				goto again;

Quoted 1 line

—

Reviewed-by Yan, Zheng <[email protected]>

Quoted 2 lines

2.5.0

Nikolay Borisov

author

2016-10-10 13:13 UTC #3 lore raw

Quoted 21 lines · Zheng

Quoted 21 lines · Nikolay Borisov
In case __ceph_do_getattr returns an error and the retry_op in ceph_read_iter is not READ_INLINE, then it's possible to invoke __free_page on a page which is NULL, this naturally leads to a crash. This can happen when, for example, a process waiting on a MDS reply receives sigterm.
Fix this by explicitly checking whether the page is set or not.
Signed-off-by Nikolay Borisov <[email protected]>
Link http://www.spinics.net/lists/ceph-users/msg31592.html
---
Inverted the condition, so resending with correct condition this time.
fs/ceph/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 16 lines

Quoted 14 lines

fs/ceph/file.c +2 -1

index 3c68e6aee2f0..7413313ae6c8 100644
@@ -929,7 +929,8 @@ again:
 		statret = __ceph_do_getattr(inode, page,
 					    CEPH_STAT_CAP_INLINE_DATA, !!page);
 		if (statret < 0) {
-			 __free_page(page);
+			if (page)
+				__free_page(page);
 			if (statret == -ENODATA) {
 				BUG_ON(retry_op != READ_INLINE);
 				goto again;

Quoted 1 line

—

Reviewed-by Yan, Zheng <[email protected]>

I believe this needs to also be tagged as stable. To whomever is going to merge it: can you please do that?

Quoted 4 lines

Quoted 3 lines
2.5.0

Yan, Zheng

2016-10-10 13:14 UTC #4 lore raw

Quoted 44 lines · Nikolay Borisov

Quoted 21 lines · Zheng
Quoted 21 lines · Nikolay Borisov
In case __ceph_do_getattr returns an error and the retry_op in ceph_read_iter is not READ_INLINE, then it's possible to invoke __free_page on a page which is NULL, this naturally leads to a crash. This can happen when, for example, a process waiting on a MDS reply receives sigterm.
Fix this by explicitly checking whether the page is set or not.
Signed-off-by Nikolay Borisov <[email protected]>
Link http://www.spinics.net/lists/ceph-users/msg31592.html
---
Inverted the condition, so resending with correct condition this time.
fs/ceph/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 16 lines
Quoted 14 lines
fs/ceph/file.c +2 -1
index 3c68e6aee2f0..7413313ae6c8 100644
@@ -929,7 +929,8 @@ again:
 		statret = __ceph_do_getattr(inode, page,
 					    CEPH_STAT_CAP_INLINE_DATA, !!page);
 		if (statret < 0) {
-			 __free_page(page);
+			if (page)
+				__free_page(page);
 			if (statret == -ENODATA) {
 				BUG_ON(retry_op != READ_INLINE);
 				goto again;
Quoted 1 line
—
Reviewed-by Yan, Zheng <[email protected]>

Quoted 3 lines

I believe this needs to also be tagged as stable. To whomever is going to merge it: can you please do that?

need to get it upstream first

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
2.5.0

Ilya Dryomov

maintainer

2016-10-10 13:50 UTC #5 lore raw

In reply to Nikolay Borisov

Quoted 41 lines · Nikolay Borisov

Quoted 21 lines · Zheng
Quoted 21 lines · Nikolay Borisov
In case __ceph_do_getattr returns an error and the retry_op in ceph_read_iter is not READ_INLINE, then it's possible to invoke __free_page on a page which is NULL, this naturally leads to a crash. This can happen when, for example, a process waiting on a MDS reply receives sigterm.
Fix this by explicitly checking whether the page is set or not.
Signed-off-by Nikolay Borisov <[email protected]>
Link http://www.spinics.net/lists/ceph-users/msg31592.html
---
Inverted the condition, so resending with correct condition this time.
fs/ceph/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 16 lines
Quoted 14 lines
fs/ceph/file.c +2 -1
index 3c68e6aee2f0..7413313ae6c8 100644
@@ -929,7 +929,8 @@ again:
             statret = __ceph_do_getattr(inode, page,
                                         CEPH_STAT_CAP_INLINE_DATA, !!page);
             if (statret < 0) {
-                     __free_page(page);
+                    if (page)
+                            __free_page(page);
                     if (statret == -ENODATA) {
                             BUG_ON(retry_op != READ_INLINE);
                             goto again;
Quoted 1 line
—
Reviewed-by Yan, Zheng <[email protected]>

Quoted 2 lines

I believe this needs to also be tagged as stable. To whomever is going to merge it: can you please do that?

I'll do that. Zheng, do you see any other issues with the killable wait here?

Thanks,

Ilya

Yan, Zheng

2016-10-10 15:09 UTC #6 lore raw

Quoted 45 lines · Ilya Dryomov

Quoted 41 lines · Nikolay Borisov
Quoted 37 lines · Zheng
Quoted 35 lines · Nikolay Borisov
In case __ceph_do_getattr returns an error and the retry_op in ceph_read_iter is not READ_INLINE, then it's possible to invoke __free_page on a page which is NULL, this naturally leads to a crash. This can happen when, for example, a process waiting on a MDS reply receives sigterm.
Fix this by explicitly checking whether the page is set or not.
Signed-off-by Nikolay Borisov <[email protected]>
Link http://www.spinics.net/lists/ceph-users/msg31592.html
---
Inverted the condition, so resending with correct condition this time.
fs/ceph/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
fs/ceph/file.c +2 -1
index 3c68e6aee2f0..7413313ae6c8 100644
@@ -929,7 +929,8 @@ again:
            statret = __ceph_do_getattr(inode, page,
                                        CEPH_STAT_CAP_INLINE_DATA, !!page);
            if (statret < 0) {
-                     __free_page(page);
+                    if (page)
+                            __free_page(page);
                    if (statret == -ENODATA) {
                            BUG_ON(retry_op != READ_INLINE);
                            goto again;
Quoted 1 line
—
Reviewed-by Yan, Zheng <[email protected]>

Quoted 5 lines

Quoted 2 lines
I believe this needs to also be tagged as stable. To whomever is going to merge it: can you please do that?
I'll do that. Zheng, do you see any other issues with the killable wait here?

No. this change is obvious, what’s your concern?

Regards
Yan, Zheng

Quoted 4 lines

Thanks,
Ilya