Message ID | 20220808070823.707829-1-xiubli@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] ceph: fail the request if the peer MDS doesn't support getvxattr op | expand |
On Mon, Aug 08, 2022 at 03:08:23PM +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > Just fail the request instead sending the request out, or the peer > MDS will crash. > > URL: https://tracker.ceph.com/issues/56529 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/inode.c | 1 + > fs/ceph/mds_client.c | 11 +++++++++++ > fs/ceph/mds_client.h | 6 +++++- > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 79ff197c7cc5..cfa0b550eef2 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -2607,6 +2607,7 @@ int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, > goto out; > } > > + req->r_feature_needed = CEPHFS_FEATURE_OP_GETVXATTR; > req->r_path2 = kstrdup(name, GFP_NOFS); > if (!req->r_path2) { > err = -ENOMEM; > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 598012ddc401..3e22783109ad 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -2501,6 +2501,7 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) > INIT_LIST_HEAD(&req->r_unsafe_dir_item); > INIT_LIST_HEAD(&req->r_unsafe_target_item); > req->r_fmode = -1; > + req->r_feature_needed = -1; > kref_init(&req->r_kref); > RB_CLEAR_NODE(&req->r_node); > INIT_LIST_HEAD(&req->r_wait); > @@ -3255,6 +3256,16 @@ static void __do_request(struct ceph_mds_client *mdsc, > > dout("do_request mds%d session %p state %s\n", mds, session, > ceph_session_state_name(session->s_state)); > + > + /* > + * The old ceph will crash the MDSs when see unknown OPs > + */ > + if (req->r_feature_needed > 0 && > + !test_bit(req->r_feature_needed, &session->s_features)) { > + err = -ENODATA; > + goto out_session; > + } This patch looks good to me. The only thing I would have preferred would be to have ->r_feature_needed defined as an unsigned and initialised to zero (instead of -1). But that's me, so feel free to ignore this nit. Cheers, -- Luís > + > if (session->s_state != CEPH_MDS_SESSION_OPEN && > session->s_state != CEPH_MDS_SESSION_HUNG) { > /* > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h > index e15ee2858fef..728b7d72bf76 100644 > --- a/fs/ceph/mds_client.h > +++ b/fs/ceph/mds_client.h > @@ -31,8 +31,9 @@ enum ceph_feature_type { > CEPHFS_FEATURE_METRIC_COLLECT, > CEPHFS_FEATURE_ALTERNATE_NAME, > CEPHFS_FEATURE_NOTIFY_SESSION_STATE, > + CEPHFS_FEATURE_OP_GETVXATTR, > > - CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_NOTIFY_SESSION_STATE, > + CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_OP_GETVXATTR, > }; > > #define CEPHFS_FEATURES_CLIENT_SUPPORTED { \ > @@ -45,6 +46,7 @@ enum ceph_feature_type { > CEPHFS_FEATURE_METRIC_COLLECT, \ > CEPHFS_FEATURE_ALTERNATE_NAME, \ > CEPHFS_FEATURE_NOTIFY_SESSION_STATE, \ > + CEPHFS_FEATURE_OP_GETVXATTR, \ > } > > /* > @@ -352,6 +354,8 @@ struct ceph_mds_request { > long long r_dir_ordered_cnt; > int r_readdir_cache_idx; > > + int r_feature_needed; > + > struct ceph_cap_reservation r_caps_reservation; > }; > > -- > 2.36.0.rc1 >
On 8/9/22 6:07 PM, Luís Henriques wrote: > On Mon, Aug 08, 2022 at 03:08:23PM +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> Just fail the request instead sending the request out, or the peer >> MDS will crash. >> >> URL: https://tracker.ceph.com/issues/56529 >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/inode.c | 1 + >> fs/ceph/mds_client.c | 11 +++++++++++ >> fs/ceph/mds_client.h | 6 +++++- >> 3 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >> index 79ff197c7cc5..cfa0b550eef2 100644 >> --- a/fs/ceph/inode.c >> +++ b/fs/ceph/inode.c >> @@ -2607,6 +2607,7 @@ int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, >> goto out; >> } >> >> + req->r_feature_needed = CEPHFS_FEATURE_OP_GETVXATTR; >> req->r_path2 = kstrdup(name, GFP_NOFS); >> if (!req->r_path2) { >> err = -ENOMEM; >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >> index 598012ddc401..3e22783109ad 100644 >> --- a/fs/ceph/mds_client.c >> +++ b/fs/ceph/mds_client.c >> @@ -2501,6 +2501,7 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) >> INIT_LIST_HEAD(&req->r_unsafe_dir_item); >> INIT_LIST_HEAD(&req->r_unsafe_target_item); >> req->r_fmode = -1; >> + req->r_feature_needed = -1; >> kref_init(&req->r_kref); >> RB_CLEAR_NODE(&req->r_node); >> INIT_LIST_HEAD(&req->r_wait); >> @@ -3255,6 +3256,16 @@ static void __do_request(struct ceph_mds_client *mdsc, >> >> dout("do_request mds%d session %p state %s\n", mds, session, >> ceph_session_state_name(session->s_state)); >> + >> + /* >> + * The old ceph will crash the MDSs when see unknown OPs >> + */ >> + if (req->r_feature_needed > 0 && >> + !test_bit(req->r_feature_needed, &session->s_features)) { >> + err = -ENODATA; >> + goto out_session; >> + } > This patch looks good to me. The only thing I would have preferred would > be to have ->r_feature_needed defined as an unsigned and initialised to > zero (instead of -1). But that's me, so feel free to ignore this nit. I am just following the 'test_bit()': 9 132 include/asm-generic/bitops/instrumented-non-atomic.h <<test_bit>> static inline bool test_bit(long nr, const volatile unsigned long *addr) 10 104 include/asm-generic/bitops/non-atomic.h <<test_bit>> static inline int test_bit(int nr, const volatile unsigned long *addr) Which is a signed type. If we use a unsigned, won't compiler complain about it ? BRs -- Xiubo > Cheers, > -- > Luís > >> + >> if (session->s_state != CEPH_MDS_SESSION_OPEN && >> session->s_state != CEPH_MDS_SESSION_HUNG) { >> /* >> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h >> index e15ee2858fef..728b7d72bf76 100644 >> --- a/fs/ceph/mds_client.h >> +++ b/fs/ceph/mds_client.h >> @@ -31,8 +31,9 @@ enum ceph_feature_type { >> CEPHFS_FEATURE_METRIC_COLLECT, >> CEPHFS_FEATURE_ALTERNATE_NAME, >> CEPHFS_FEATURE_NOTIFY_SESSION_STATE, >> + CEPHFS_FEATURE_OP_GETVXATTR, >> >> - CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_NOTIFY_SESSION_STATE, >> + CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_OP_GETVXATTR, >> }; >> >> #define CEPHFS_FEATURES_CLIENT_SUPPORTED { \ >> @@ -45,6 +46,7 @@ enum ceph_feature_type { >> CEPHFS_FEATURE_METRIC_COLLECT, \ >> CEPHFS_FEATURE_ALTERNATE_NAME, \ >> CEPHFS_FEATURE_NOTIFY_SESSION_STATE, \ >> + CEPHFS_FEATURE_OP_GETVXATTR, \ >> } >> >> /* >> @@ -352,6 +354,8 @@ struct ceph_mds_request { >> long long r_dir_ordered_cnt; >> int r_readdir_cache_idx; >> >> + int r_feature_needed; >> + >> struct ceph_cap_reservation r_caps_reservation; >> }; >> >> -- >> 2.36.0.rc1 >>
On Tue, Aug 09, 2022 at 06:16:36PM +0800, Xiubo Li wrote: > > On 8/9/22 6:07 PM, Luís Henriques wrote: > > On Mon, Aug 08, 2022 at 03:08:23PM +0800, xiubli@redhat.com wrote: > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > Just fail the request instead sending the request out, or the peer > > > MDS will crash. > > > > > > URL: https://tracker.ceph.com/issues/56529 > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > > --- > > > fs/ceph/inode.c | 1 + > > > fs/ceph/mds_client.c | 11 +++++++++++ > > > fs/ceph/mds_client.h | 6 +++++- > > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > > index 79ff197c7cc5..cfa0b550eef2 100644 > > > --- a/fs/ceph/inode.c > > > +++ b/fs/ceph/inode.c > > > @@ -2607,6 +2607,7 @@ int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, > > > goto out; > > > } > > > + req->r_feature_needed = CEPHFS_FEATURE_OP_GETVXATTR; > > > req->r_path2 = kstrdup(name, GFP_NOFS); > > > if (!req->r_path2) { > > > err = -ENOMEM; > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > > index 598012ddc401..3e22783109ad 100644 > > > --- a/fs/ceph/mds_client.c > > > +++ b/fs/ceph/mds_client.c > > > @@ -2501,6 +2501,7 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) > > > INIT_LIST_HEAD(&req->r_unsafe_dir_item); > > > INIT_LIST_HEAD(&req->r_unsafe_target_item); > > > req->r_fmode = -1; > > > + req->r_feature_needed = -1; > > > kref_init(&req->r_kref); > > > RB_CLEAR_NODE(&req->r_node); > > > INIT_LIST_HEAD(&req->r_wait); > > > @@ -3255,6 +3256,16 @@ static void __do_request(struct ceph_mds_client *mdsc, > > > dout("do_request mds%d session %p state %s\n", mds, session, > > > ceph_session_state_name(session->s_state)); > > > + > > > + /* > > > + * The old ceph will crash the MDSs when see unknown OPs > > > + */ > > > + if (req->r_feature_needed > 0 && > > > + !test_bit(req->r_feature_needed, &session->s_features)) { > > > + err = -ENODATA; > > > + goto out_session; > > > + } > > This patch looks good to me. The only thing I would have preferred would > > be to have ->r_feature_needed defined as an unsigned and initialised to > > zero (instead of -1). But that's me, so feel free to ignore this nit. > > I am just following the 'test_bit()': > > 9 132 include/asm-generic/bitops/instrumented-non-atomic.h > <<test_bit>> > static inline bool test_bit(long nr, const volatile unsigned > long *addr) > 10 104 include/asm-generic/bitops/non-atomic.h <<test_bit>> > static inline int test_bit(int nr, const volatile unsigned long > *addr) > > Which is a signed type. If we use a unsigned, won't compiler complain about > it ? > Oh, yeah you're right. And now that I think about it, I think I've been there already in the past. Sorry for the noise. Cheers, -- Luís > BRs > > -- Xiubo > > > Cheers, > > -- > > Luís > > > > > + > > > if (session->s_state != CEPH_MDS_SESSION_OPEN && > > > session->s_state != CEPH_MDS_SESSION_HUNG) { > > > /* > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h > > > index e15ee2858fef..728b7d72bf76 100644 > > > --- a/fs/ceph/mds_client.h > > > +++ b/fs/ceph/mds_client.h > > > @@ -31,8 +31,9 @@ enum ceph_feature_type { > > > CEPHFS_FEATURE_METRIC_COLLECT, > > > CEPHFS_FEATURE_ALTERNATE_NAME, > > > CEPHFS_FEATURE_NOTIFY_SESSION_STATE, > > > + CEPHFS_FEATURE_OP_GETVXATTR, > > > - CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_NOTIFY_SESSION_STATE, > > > + CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_OP_GETVXATTR, > > > }; > > > #define CEPHFS_FEATURES_CLIENT_SUPPORTED { \ > > > @@ -45,6 +46,7 @@ enum ceph_feature_type { > > > CEPHFS_FEATURE_METRIC_COLLECT, \ > > > CEPHFS_FEATURE_ALTERNATE_NAME, \ > > > CEPHFS_FEATURE_NOTIFY_SESSION_STATE, \ > > > + CEPHFS_FEATURE_OP_GETVXATTR, \ > > > } > > > /* > > > @@ -352,6 +354,8 @@ struct ceph_mds_request { > > > long long r_dir_ordered_cnt; > > > int r_readdir_cache_idx; > > > + int r_feature_needed; > > > + > > > struct ceph_cap_reservation r_caps_reservation; > > > }; > > > -- > > > 2.36.0.rc1 > > > >
On 8/9/22 8:53 PM, Luís Henriques wrote: > On Tue, Aug 09, 2022 at 06:16:36PM +0800, Xiubo Li wrote: >> On 8/9/22 6:07 PM, Luís Henriques wrote: >>> On Mon, Aug 08, 2022 at 03:08:23PM +0800, xiubli@redhat.com wrote: >>>> From: Xiubo Li <xiubli@redhat.com> >>>> >>>> Just fail the request instead sending the request out, or the peer >>>> MDS will crash. >>>> >>>> URL: https://tracker.ceph.com/issues/56529 >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>> --- >>>> fs/ceph/inode.c | 1 + >>>> fs/ceph/mds_client.c | 11 +++++++++++ >>>> fs/ceph/mds_client.h | 6 +++++- >>>> 3 files changed, 17 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >>>> index 79ff197c7cc5..cfa0b550eef2 100644 >>>> --- a/fs/ceph/inode.c >>>> +++ b/fs/ceph/inode.c >>>> @@ -2607,6 +2607,7 @@ int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, >>>> goto out; >>>> } >>>> + req->r_feature_needed = CEPHFS_FEATURE_OP_GETVXATTR; >>>> req->r_path2 = kstrdup(name, GFP_NOFS); >>>> if (!req->r_path2) { >>>> err = -ENOMEM; >>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >>>> index 598012ddc401..3e22783109ad 100644 >>>> --- a/fs/ceph/mds_client.c >>>> +++ b/fs/ceph/mds_client.c >>>> @@ -2501,6 +2501,7 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) >>>> INIT_LIST_HEAD(&req->r_unsafe_dir_item); >>>> INIT_LIST_HEAD(&req->r_unsafe_target_item); >>>> req->r_fmode = -1; >>>> + req->r_feature_needed = -1; >>>> kref_init(&req->r_kref); >>>> RB_CLEAR_NODE(&req->r_node); >>>> INIT_LIST_HEAD(&req->r_wait); >>>> @@ -3255,6 +3256,16 @@ static void __do_request(struct ceph_mds_client *mdsc, >>>> dout("do_request mds%d session %p state %s\n", mds, session, >>>> ceph_session_state_name(session->s_state)); >>>> + >>>> + /* >>>> + * The old ceph will crash the MDSs when see unknown OPs >>>> + */ >>>> + if (req->r_feature_needed > 0 && >>>> + !test_bit(req->r_feature_needed, &session->s_features)) { >>>> + err = -ENODATA; >>>> + goto out_session; >>>> + } >>> This patch looks good to me. The only thing I would have preferred would >>> be to have ->r_feature_needed defined as an unsigned and initialised to >>> zero (instead of -1). But that's me, so feel free to ignore this nit. >> I am just following the 'test_bit()': >> >> 9 132 include/asm-generic/bitops/instrumented-non-atomic.h >> <<test_bit>> >> static inline bool test_bit(long nr, const volatile unsigned >> long *addr) >> 10 104 include/asm-generic/bitops/non-atomic.h <<test_bit>> >> static inline int test_bit(int nr, const volatile unsigned long >> *addr) >> >> Which is a signed type. If we use a unsigned, won't compiler complain about >> it ? >> > Oh, yeah you're right. And now that I think about it, I think I've been > there already in the past. Sorry for the noise. No worry about that. Thanks very much Luis for your reviewing of this ! -- Xiubo > Cheers, > -- > Luís > >> BRs >> >> -- Xiubo >> >>> Cheers, >>> -- >>> Luís >>> >>>> + >>>> if (session->s_state != CEPH_MDS_SESSION_OPEN && >>>> session->s_state != CEPH_MDS_SESSION_HUNG) { >>>> /* >>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h >>>> index e15ee2858fef..728b7d72bf76 100644 >>>> --- a/fs/ceph/mds_client.h >>>> +++ b/fs/ceph/mds_client.h >>>> @@ -31,8 +31,9 @@ enum ceph_feature_type { >>>> CEPHFS_FEATURE_METRIC_COLLECT, >>>> CEPHFS_FEATURE_ALTERNATE_NAME, >>>> CEPHFS_FEATURE_NOTIFY_SESSION_STATE, >>>> + CEPHFS_FEATURE_OP_GETVXATTR, >>>> - CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_NOTIFY_SESSION_STATE, >>>> + CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_OP_GETVXATTR, >>>> }; >>>> #define CEPHFS_FEATURES_CLIENT_SUPPORTED { \ >>>> @@ -45,6 +46,7 @@ enum ceph_feature_type { >>>> CEPHFS_FEATURE_METRIC_COLLECT, \ >>>> CEPHFS_FEATURE_ALTERNATE_NAME, \ >>>> CEPHFS_FEATURE_NOTIFY_SESSION_STATE, \ >>>> + CEPHFS_FEATURE_OP_GETVXATTR, \ >>>> } >>>> /* >>>> @@ -352,6 +354,8 @@ struct ceph_mds_request { >>>> long long r_dir_ordered_cnt; >>>> int r_readdir_cache_idx; >>>> + int r_feature_needed; >>>> + >>>> struct ceph_cap_reservation r_caps_reservation; >>>> }; >>>> -- >>>> 2.36.0.rc1 >>>>
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 79ff197c7cc5..cfa0b550eef2 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -2607,6 +2607,7 @@ int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, goto out; } + req->r_feature_needed = CEPHFS_FEATURE_OP_GETVXATTR; req->r_path2 = kstrdup(name, GFP_NOFS); if (!req->r_path2) { err = -ENOMEM; diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 598012ddc401..3e22783109ad 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2501,6 +2501,7 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) INIT_LIST_HEAD(&req->r_unsafe_dir_item); INIT_LIST_HEAD(&req->r_unsafe_target_item); req->r_fmode = -1; + req->r_feature_needed = -1; kref_init(&req->r_kref); RB_CLEAR_NODE(&req->r_node); INIT_LIST_HEAD(&req->r_wait); @@ -3255,6 +3256,16 @@ static void __do_request(struct ceph_mds_client *mdsc, dout("do_request mds%d session %p state %s\n", mds, session, ceph_session_state_name(session->s_state)); + + /* + * The old ceph will crash the MDSs when see unknown OPs + */ + if (req->r_feature_needed > 0 && + !test_bit(req->r_feature_needed, &session->s_features)) { + err = -ENODATA; + goto out_session; + } + if (session->s_state != CEPH_MDS_SESSION_OPEN && session->s_state != CEPH_MDS_SESSION_HUNG) { /* diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h index e15ee2858fef..728b7d72bf76 100644 --- a/fs/ceph/mds_client.h +++ b/fs/ceph/mds_client.h @@ -31,8 +31,9 @@ enum ceph_feature_type { CEPHFS_FEATURE_METRIC_COLLECT, CEPHFS_FEATURE_ALTERNATE_NAME, CEPHFS_FEATURE_NOTIFY_SESSION_STATE, + CEPHFS_FEATURE_OP_GETVXATTR, - CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_NOTIFY_SESSION_STATE, + CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_OP_GETVXATTR, }; #define CEPHFS_FEATURES_CLIENT_SUPPORTED { \ @@ -45,6 +46,7 @@ enum ceph_feature_type { CEPHFS_FEATURE_METRIC_COLLECT, \ CEPHFS_FEATURE_ALTERNATE_NAME, \ CEPHFS_FEATURE_NOTIFY_SESSION_STATE, \ + CEPHFS_FEATURE_OP_GETVXATTR, \ } /* @@ -352,6 +354,8 @@ struct ceph_mds_request { long long r_dir_ordered_cnt; int r_readdir_cache_idx; + int r_feature_needed; + struct ceph_cap_reservation r_caps_reservation; };