[PATCH] ceph: fix race in concurrent __ceph_remove_cap invocations

[PATCH] ceph: fix race in concurrent __ceph_remove_cap invocations

[Luis Henriques]

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: stable@vger.kernel.org
URL: https://tracker.ceph.com/issues/43272
Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
 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) {

Re: [PATCH] ceph: fix race in concurrent __ceph_remove_cap invocations

[Jeff Layton]

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: stable@vger.kernel.org
> URL: https://tracker.ceph.com/issues/43272
> Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  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 <jlayton@kernel.org>

Re: [PATCH] ceph: fix race in concurrent __ceph_remove_cap invocations

[Yan, Zheng]

On Thu, Nov 12, 2020 at 6:48 PM Luis Henriques <lhenriques@suse.de> 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: stable@vger.kernel.org
> URL: https://tracker.ceph.com/issues/43272
> Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
>  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) {

Re: [PATCH] ceph: fix race in concurrent __ceph_remove_cap invocations

[Jeff Layton]

On Thu, 2020-11-12 at 20:43 +0800, Yan, Zheng wrote:
> On Thu, Nov 12, 2020 at 6:48 PM Luis Henriques <lhenriques@suse.de> 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: stable@vger.kernel.org
> > URL: https://tracker.ceph.com/issues/43272
> > Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
> > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > ---
> >  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 <jlayton@kernel.org>

Re: [PATCH] ceph: fix race in concurrent __ceph_remove_cap invocations

[Luis Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> On Thu, 2020-11-12 at 20:43 +0800, Yan, Zheng wrote:
>> On Thu, Nov 12, 2020 at 6:48 PM Luis Henriques <lhenriques@suse.de> 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: stable@vger.kernel.org
>> > URL: https://tracker.ceph.com/issues/43272
>> > Link: https://www.spinics.net/lists/ceph-devel/msg47064.html
>> > Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> > ---
>> >  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 <jlayton@kernel.org>
>