Messages in this thread | | | Date | Fri, 17 Nov 2023 16:53:49 +0800 | Subject | Re: [PATCH] ceph: quota: Fix invalid pointer access in | From | Wenchao Hao <> |
| |
On 2023/11/17 14:14, Xiubo Li wrote: > > On 11/16/23 15:09, Wenchao Hao wrote: >> On 2023/11/16 11:06, Xiubo Li wrote: >>> >>> On 11/16/23 10:54, Wenchao Hao wrote: >>>> On 2023/11/15 21:34, Xiubo Li wrote: >>>>> >>>>> On 11/15/23 21:25, Ilya Dryomov wrote: >>>>>> On Wed, Nov 15, 2023 at 2:17 PM Xiubo Li <xiubli@redhat.com> wrote: >>>>>>> >>>>>>> On 11/15/23 20:32, Ilya Dryomov wrote: >>>>>>>> On Wed, Nov 15, 2023 at 1:35 AM Xiubo Li <xiubli@redhat.com> wrote: >>>>>>>>> On 11/14/23 23:31, Wenchao Hao wrote: >>>>>>>>>> This issue is reported by smatch, get_quota_realm() might return >>>>>>>>>> ERR_PTR, so we should using IS_ERR_OR_NULL here to check the return >>>>>>>>>> value. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Wenchao Hao <haowenchao2@huawei.com> >>>>>>>>>> --- >>>>>>>>>> fs/ceph/quota.c | 2 +- >>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c >>>>>>>>>> index 9d36c3532de1..c4b2929c6a83 100644 >>>>>>>>>> --- a/fs/ceph/quota.c >>>>>>>>>> +++ b/fs/ceph/quota.c >>>>>>>>>> @@ -495,7 +495,7 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf) >>>>>>>>>> realm = get_quota_realm(mdsc, d_inode(fsc->sb->s_root), >>>>>>>>>> QUOTA_GET_MAX_BYTES, true); >>>>>>>>>> up_read(&mdsc->snap_rwsem); >>>>>>>>>> - if (!realm) >>>>>>>>>> + if (IS_ERR_OR_NULL(realm)) >>>>>>>>>> return false; >>>>>>>>>> >>>>>>>>>> spin_lock(&realm->inodes_with_caps_lock); >>>>>>>>> Good catch. >>>>>>>>> >>>>>>>>> Reviewed-by: Xiubo Li <xiubli@redhat.com> >>>>>>>>> >>>>>>>>> We should CC the stable mail list. >>>>>>>> Hi Xiubo, >>>>>>>> >>>>>>>> What exactly is being fixed here? get_quota_realm() is called with >>>>>>>> retry=true, which means that no errors can be returned -- EAGAIN, the >>>>>>>> only error that get_quota_realm() can otherwise generate, would be >>>>>>>> handled internally by retrying. >>>>>>> Yeah, that's true. >>>>>>> >>>>>>>> Am I missing something that makes this qualify for stable? >>>>>>> Actually it's just for the smatch check for now. >>>>>>> >>>>>>> IMO we shouldn't depend on the 'retry', just potentially for new changes >>>>>>> in future could return a ERR_PTR and cause potential bugs. >>>>>> At present, ceph_quota_is_same_realm() also depends on it -- note how >>>>>> old_realm isn't checked for errors at all and new_realm is only checked >>>>>> for EAGAIN there. >>>>>> >>>>>>> If that's not worth to make it for stable, let's remove it. >>>>>> Yes, let's remove it. Please update the commit message as well, so >>>>>> that it's clear that this is squashing a static checker warning and >>>>>> doesn't actually fix any immediate bug. >>>>> >>>>> WenChao, >>>>> >>>>> Could update the commit comment and send the V2 ? >>>>> >>>> >>>> OK, I would update the commit comment as following: >>>> >>>> This issue is reported by smatch, get_quota_realm() might return >>>> ERR_PTR. It's not a immediate bug because get_quota_realm() is called >>>> with 'retry=true', no errors can be returned. >>>> >>>> While we still should check the return value of get_quota_realm() with >>>> IS_ERR_OR_NULL to avoid potential bugs if get_quota_realm() is changed >>>> to return other ERR_PTR in future. >>>> >>>> What's more, should I change the ceph_quota_is_same_realm() too? >>>> >>> Yeah, please. Let's fix them all. >>> >> >> is_same is return as true if both old_realm and new_realm are NULL, I do not >> want to change the origin logic except add check for ERR_PTR, so following >> is my change: >> >> 1. make sure xxx_realm is valid before calling ceph_put_snap_realm. >> 2. return false if new_realm or old_realm is ERR_PTR, this is newly added >> and now we would always run with the else branch. >> >> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c >> index c4b2929c6a83..8da9ffb05395 100644 >> --- a/fs/ceph/quota.c >> +++ b/fs/ceph/quota.c >> @@ -290,16 +290,20 @@ bool ceph_quota_is_same_realm(struct inode *old, struct inode *new) >> new_realm = get_quota_realm(mdsc, new, QUOTA_GET_ANY, false); >> if (PTR_ERR(new_realm) == -EAGAIN) { >> up_read(&mdsc->snap_rwsem); >> - if (old_realm) >> + if (!IS_ERR_OR_NULL(old_realm)) >> ceph_put_snap_realm(mdsc, old_realm); >> goto restart; >> } >> - is_same = (old_realm == new_realm); >> up_read(&mdsc->snap_rwsem); >> >> - if (old_realm) >> + if (IS_ERR(new_realm)) >> + is_same = false; >> + else >> + is_same = (old_realm == new_realm); >> + >> + if (!IS_ERR_OR_NULL(old_realm)) >> ceph_put_snap_realm(mdsc, old_realm); >> - if (new_realm) >> + if (!IS_ERR_OR_NULL(new_realm)) >> ceph_put_snap_realm(mdsc, new_realm); >> >> return is_same; >> > If we just to fix the smatch check, how about make get_quota_realm() to return a 'int' type value and at the same time add a 'realmp' parameter ? And just return '-EAGAIN' or '0' always. > > Then it will be something likes: > > > diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c > index c4b2929c6a83..f37f5324b6a1 100644 > --- a/fs/ceph/quota.c > +++ b/fs/ceph/quota.c > @@ -211,10 +211,9 @@ void ceph_cleanup_quotarealms_inodes(struct ceph_mds_client *mdsc) > * this function will return -EAGAIN; otherwise, the snaprealms walk-through > * will be restarted. > */ > -static struct ceph_snap_realm *get_quota_realm(struct ceph_mds_client *mdsc, > - struct inode *inode, > - enum quota_get_realm which_quota, > - bool retry) > +static int get_quota_realm(struct ceph_mds_client *mdsc, struct inode *inode, > + enum quota_get_realm which_quota, > + struct ceph_snap_realm **realmp, bool retry) > { > struct ceph_client *cl = mdsc->fsc->client; > struct ceph_inode_info *ci = NULL; > @@ -222,8 +221,10 @@ static struct ceph_snap_realm *get_quota_realm(struct ceph_mds_client *mdsc, > struct inode *in; > bool has_quota; > > + if (realmp) > + *realmp = NULL; > if (ceph_snap(inode) != CEPH_NOSNAP) > - return NULL; > + return 0; > > restart: > realm = ceph_inode(inode)->i_snap_realm; > @@ -250,7 +251,7 @@ static struct ceph_snap_realm *get_quota_realm(struct ceph_mds_client *mdsc, > break; > ceph_put_snap_realm(mdsc, realm); > if (!retry) > - return ERR_PTR(-EAGAIN); > + return -EAGAIN; > goto restart; > } > > @@ -259,8 +260,11 @@ static struct ceph_snap_realm *get_quota_realm(struct ceph_mds_client *mdsc, > iput(in); > > next = realm->parent; > - if (has_quota || !next) > - return realm; > + if (has_quota || !next) { > + if (realmp) > + *realmp = realm; > + return 0; > + } > > ceph_get_snap_realm(mdsc, next); > ceph_put_snap_realm(mdsc, realm); > @@ -269,14 +273,15 @@ static struct ceph_snap_realm *get_quota_realm(struct ceph_mds_client *mdsc, > if (realm) > ceph_put_snap_realm(mdsc, realm); > > - return NULL; > + return 0; > } > > bool ceph_quota_is_same_realm(struct inode *old, struct inode *new) > { > struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(old->i_sb); > - struct ceph_snap_realm *old_realm, *new_realm; > + struct ceph_snap_realm *old_realm = NULL, *new_realm = NULL; > bool is_same; > + int ret; > > restart: > /* > @@ -286,9 +291,9 @@ bool ceph_quota_is_same_realm(struct inode *old, struct inode *new) > * dropped and we can then restart the whole operation. > */ > down_read(&mdsc->snap_rwsem); > - old_realm = get_quota_realm(mdsc, old, QUOTA_GET_ANY, true); > - new_realm = get_quota_realm(mdsc, new, QUOTA_GET_ANY, false); > - if (PTR_ERR(new_realm) == -EAGAIN) { > + get_quota_realm(mdsc, old, QUOTA_GET_ANY, &old_relam, true); > + ret = get_quota_realm(mdsc, new, QUOTA_GET_ANY, &new_realm, false); > + if (ret == -EAGAIN) { > up_read(&mdsc->snap_rwsem); > if (old_realm) > ceph_put_snap_realm(mdsc, old_realm); > > > Won't be this better ? >
Yes, it looks better.
Would you post a patch to address it? Or should I apply your changes and send a V2?
> Thanks > > - Xiubo > > > > >> >>> Thanks >>> >>> - Xiubo >>> >>> >>>> Thanks >>>> >>>>> Thanks >>>>> >>>>> - Xiubo >>>>> >>>>> >>>>>> Thanks, >>>>>> >>>>>> Ilya >>>>>> >>>>> >>>> >>> >>> >> >
| |