A NULL pointer dereference may occur in __ceph_remove_cap with some of the
callbacks used in ceph_iterate_session_caps, namely trim_caps_cb and
remove_session_caps_cb. These aren't protected against the concurrent
execution of __ceph_remove_cap.
Since the callers of this function hold the i_ceph_lock, the fix is simply
a matter of returning immediately if caps->ci is NULL.
Based on a patch from Jeff Layton.
Cc: [email protected]
URL: https://tracker.ceph.com/issues/43272
Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
Signed-off-by: Luis Henriques <[email protected]>
---
fs/ceph/caps.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index ded4229c314a..443f164760d5 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -1140,12 +1140,19 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
{
struct ceph_mds_session *session = cap->session;
struct ceph_inode_info *ci = cap->ci;
- struct ceph_mds_client *mdsc =
- ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc;
+ struct ceph_mds_client *mdsc;
int removed = 0;
+ /* 'ci' being NULL means he remove have already occurred */
+ if (!ci) {
+ dout("%s: cap inode is NULL\n", __func__);
+ return;
+ }
+
dout("__ceph_remove_cap %p from %p\n", cap, &ci->vfs_inode);
+ mdsc = ceph_inode_to_client(&ci->vfs_inode)->mdsc;
+
/* remove from inode's cap rbtree, and clear auth cap */
rb_erase(&cap->ci_node, &ci->i_caps);
if (ci->i_auth_cap == cap) {
On Thu, 2020-11-12 at 10:45 +0000, Luis Henriques wrote:
> A NULL pointer dereference may occur in __ceph_remove_cap with some of the
> callbacks used in ceph_iterate_session_caps, namely trim_caps_cb and
> remove_session_caps_cb. These aren't protected against the concurrent
> execution of __ceph_remove_cap.
>
> Since the callers of this function hold the i_ceph_lock, the fix is simply
> a matter of returning immediately if caps->ci is NULL.
>
> Based on a patch from Jeff Layton.
>
> Cc: [email protected]
> URL: https://tracker.ceph.com/issues/43272
> Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
> Signed-off-by: Luis Henriques <[email protected]>
> ---
> fs/ceph/caps.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index ded4229c314a..443f164760d5 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -1140,12 +1140,19 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
> {
> struct ceph_mds_session *session = cap->session;
> struct ceph_inode_info *ci = cap->ci;
> - struct ceph_mds_client *mdsc =
> - ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc;
> + struct ceph_mds_client *mdsc;
> int removed = 0;
>
>
> + /* 'ci' being NULL means he remove have already occurred */
> + if (!ci) {
> + dout("%s: cap inode is NULL\n", __func__);
> + return;
> + }
> +
> dout("__ceph_remove_cap %p from %p\n", cap, &ci->vfs_inode);
>
>
> + mdsc = ceph_inode_to_client(&ci->vfs_inode)->mdsc;
> +
> /* remove from inode's cap rbtree, and clear auth cap */
> rb_erase(&cap->ci_node, &ci->i_caps);
> if (ci->i_auth_cap == cap) {
Merged into testing branch (with a minor fix to the comment).
--
Jeff Layton <[email protected]>
On Thu, Nov 12, 2020 at 6:48 PM Luis Henriques <[email protected]> wrote:
>
> A NULL pointer dereference may occur in __ceph_remove_cap with some of the
> callbacks used in ceph_iterate_session_caps, namely trim_caps_cb and
> remove_session_caps_cb. These aren't protected against the concurrent
> execution of __ceph_remove_cap.
>
they are protected by session mutex, never get executed concurrently
> Since the callers of this function hold the i_ceph_lock, the fix is simply
> a matter of returning immediately if caps->ci is NULL.
>
> Based on a patch from Jeff Layton.
>
> Cc: [email protected]
> URL: https://tracker.ceph.com/issues/43272
> Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
> Signed-off-by: Luis Henriques <[email protected]>
> ---
> fs/ceph/caps.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index ded4229c314a..443f164760d5 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -1140,12 +1140,19 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
> {
> struct ceph_mds_session *session = cap->session;
> struct ceph_inode_info *ci = cap->ci;
> - struct ceph_mds_client *mdsc =
> - ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc;
> + struct ceph_mds_client *mdsc;
> int removed = 0;
>
> + /* 'ci' being NULL means he remove have already occurred */
> + if (!ci) {
> + dout("%s: cap inode is NULL\n", __func__);
> + return;
> + }
> +
> dout("__ceph_remove_cap %p from %p\n", cap, &ci->vfs_inode);
>
> + mdsc = ceph_inode_to_client(&ci->vfs_inode)->mdsc;
> +
> /* remove from inode's cap rbtree, and clear auth cap */
> rb_erase(&cap->ci_node, &ci->i_caps);
> if (ci->i_auth_cap == cap) {
On Thu, 2020-11-12 at 20:43 +0800, Yan, Zheng wrote:
> On Thu, Nov 12, 2020 at 6:48 PM Luis Henriques <[email protected]> wrote:
> >
> > A NULL pointer dereference may occur in __ceph_remove_cap with some of the
> > callbacks used in ceph_iterate_session_caps, namely trim_caps_cb and
> > remove_session_caps_cb. These aren't protected against the concurrent
> > execution of __ceph_remove_cap.
> >
>
> they are protected by session mutex, never get executed concurrently
>
Maybe not concurrently with one another, but the s_mutex is _not_ held
when __ceph_remove_caps is called from ceph_evict_inode. We can't rely
on it to protect this.
> > Since the callers of this function hold the i_ceph_lock, the fix is simply
> > a matter of returning immediately if caps->ci is NULL.
> >
> > Based on a patch from Jeff Layton.
> >
> > Cc: [email protected]
> > URL: https://tracker.ceph.com/issues/43272
> > Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
> > Signed-off-by: Luis Henriques <[email protected]>
> > ---
> > fs/ceph/caps.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index ded4229c314a..443f164760d5 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -1140,12 +1140,19 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
> > {
> > struct ceph_mds_session *session = cap->session;
> > struct ceph_inode_info *ci = cap->ci;
> > - struct ceph_mds_client *mdsc =
> > - ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc;
> > + struct ceph_mds_client *mdsc;
> > int removed = 0;
> >
> > + /* 'ci' being NULL means he remove have already occurred */
> > + if (!ci) {
> > + dout("%s: cap inode is NULL\n", __func__);
> > + return;
> > + }
> > +
> > dout("__ceph_remove_cap %p from %p\n", cap, &ci->vfs_inode);
> >
> > + mdsc = ceph_inode_to_client(&ci->vfs_inode)->mdsc;
> > +
> > /* remove from inode's cap rbtree, and clear auth cap */
> > rb_erase(&cap->ci_node, &ci->i_caps);
> > if (ci->i_auth_cap == cap) {
--
Jeff Layton <[email protected]>
Jeff Layton <[email protected]> writes:
> On Thu, 2020-11-12 at 20:43 +0800, Yan, Zheng wrote:
>> On Thu, Nov 12, 2020 at 6:48 PM Luis Henriques <[email protected]> wrote:
>> >
>> > A NULL pointer dereference may occur in __ceph_remove_cap with some of the
>> > callbacks used in ceph_iterate_session_caps, namely trim_caps_cb and
>> > remove_session_caps_cb. These aren't protected against the concurrent
>> > execution of __ceph_remove_cap.
>> >
>>
>> they are protected by session mutex, never get executed concurrently
>>
>
> Maybe not concurrently with one another, but the s_mutex is _not_ held
> when __ceph_remove_caps is called from ceph_evict_inode. We can't rely
> on it to protect this.
Hmm, yeah. I guess the changelog could mention that. Thanks, Jeff.
Cheers,
--
Luis
>> > Since the callers of this function hold the i_ceph_lock, the fix is simply
>> > a matter of returning immediately if caps->ci is NULL.
>> >
>> > Based on a patch from Jeff Layton.
>> >
>> > Cc: [email protected]
>> > URL: https://tracker.ceph.com/issues/43272
>> > Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
>> > Signed-off-by: Luis Henriques <[email protected]>
>> > ---
>> > fs/ceph/caps.c | 11 +++++++++--
>> > 1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>> > index ded4229c314a..443f164760d5 100644
>> > --- a/fs/ceph/caps.c
>> > +++ b/fs/ceph/caps.c
>> > @@ -1140,12 +1140,19 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
>> > {
>> > struct ceph_mds_session *session = cap->session;
>> > struct ceph_inode_info *ci = cap->ci;
>> > - struct ceph_mds_client *mdsc =
>> > - ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc;
>> > + struct ceph_mds_client *mdsc;
>> > int removed = 0;
>> >
>> > + /* 'ci' being NULL means he remove have already occurred */
>> > + if (!ci) {
>> > + dout("%s: cap inode is NULL\n", __func__);
>> > + return;
>> > + }
>> > +
>> > dout("__ceph_remove_cap %p from %p\n", cap, &ci->vfs_inode);
>> >
>> > + mdsc = ceph_inode_to_client(&ci->vfs_inode)->mdsc;
>> > +
>> > /* remove from inode's cap rbtree, and clear auth cap */
>> > rb_erase(&cap->ci_node, &ci->i_caps);
>> > if (ci->i_auth_cap == cap) {
>
> --
> Jeff Layton <[email protected]>
>