| Message ID | 20231120082637.2717550-1-haowenchao2@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ceph: quota: Fix invalid pointer access if get_quota_realm return ERR_PTR | expand |
On 11/20/23 16:26, Wenchao Hao wrote: > This issue is reported by smatch that get_quota_realm() might return > ERR_PTR but we did not handle it. It's not a immediate bug, while we > still should address it to avoid potential bugs if get_quota_realm() > is changed to return other ERR_PTR in future. > > Set ceph_snap_realm's pointer in get_quota_realm()'s to address this > issue, the pointer would be set to NULL if get_quota_realm() failed > to get struct ceph_snap_realm, so no ERR_PTR would happen any more. > > Signed-off-by: Wenchao Hao <haowenchao2@huawei.com> > --- > V2: > - Fix all potential invalid pointer access caused by get_quota_realm > - Update commit comment and point it's not a immediate bug > > fs/ceph/quota.c | 41 +++++++++++++++++++++++------------------ > 1 file changed, 23 insertions(+), 18 deletions(-) > > diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c > index 9d36c3532de1..e85a85b34a83 100644 > --- a/fs/ceph/quota.c > +++ b/fs/ceph/quota.c > @@ -197,10 +197,10 @@ void ceph_cleanup_quotarealms_inodes(struct ceph_mds_client *mdsc) > } > > /* > - * This function walks through the snaprealm for an inode and returns the > - * ceph_snap_realm for the first snaprealm that has quotas set (max_files, > + * This function walks through the snaprealm for an inode and set the > + * realmp with the first snaprealm that has quotas set (max_files, > * max_bytes, or any, depending on the 'which_quota' argument). If the root is > - * reached, return the root ceph_snap_realm instead. > + * reached, set the realmp with the root ceph_snap_realm instead. > * > * Note that the caller is responsible for calling ceph_put_snap_realm() on the > * returned realm. > @@ -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; Here you initialized it as 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_realm, 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); > @@ -492,8 +497,8 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf) > bool is_updated = false; > > down_read(&mdsc->snap_rwsem); > - realm = get_quota_realm(mdsc, d_inode(fsc->sb->s_root), > - QUOTA_GET_MAX_BYTES, true); > + get_quota_realm(mdsc, d_inode(fsc->sb->s_root), > + QUOTA_GET_MAX_BYTES, &realm, true); While here you ignored it. I think we can just ignore it and the 'get_quota_realm()' will help initialize it. Thanks - Xiubo > up_read(&mdsc->snap_rwsem); > if (!realm) > return false;
On 2023/11/22 12:29, Xiubo Li wrote: > > On 11/20/23 16:26, Wenchao Hao wrote: >> This issue is reported by smatch that get_quota_realm() might return >> ERR_PTR but we did not handle it. It's not a immediate bug, while we >> still should address it to avoid potential bugs if get_quota_realm() >> is changed to return other ERR_PTR in future. >> >> Set ceph_snap_realm's pointer in get_quota_realm()'s to address this >> issue, the pointer would be set to NULL if get_quota_realm() failed >> to get struct ceph_snap_realm, so no ERR_PTR would happen any more. >> >> Signed-off-by: Wenchao Hao <haowenchao2@huawei.com> >> --- >> V2: >> - Fix all potential invalid pointer access caused by get_quota_realm >> - Update commit comment and point it's not a immediate bug >> >> fs/ceph/quota.c | 41 +++++++++++++++++++++++------------------ >> 1 file changed, 23 insertions(+), 18 deletions(-) >> >> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c >> index 9d36c3532de1..e85a85b34a83 100644 >> --- a/fs/ceph/quota.c >> +++ b/fs/ceph/quota.c >> @@ -197,10 +197,10 @@ void ceph_cleanup_quotarealms_inodes(struct ceph_mds_client *mdsc) >> } >> /* >> - * This function walks through the snaprealm for an inode and returns the >> - * ceph_snap_realm for the first snaprealm that has quotas set (max_files, >> + * This function walks through the snaprealm for an inode and set the >> + * realmp with the first snaprealm that has quotas set (max_files, >> * max_bytes, or any, depending on the 'which_quota' argument). If the root is >> - * reached, return the root ceph_snap_realm instead. >> + * reached, set the realmp with the root ceph_snap_realm instead. >> * >> * Note that the caller is responsible for calling ceph_put_snap_realm() on the >> * returned realm. >> @@ -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; > > Here you initialized it as 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_realm, 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); >> @@ -492,8 +497,8 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf) >> bool is_updated = false; >> down_read(&mdsc->snap_rwsem); >> - realm = get_quota_realm(mdsc, d_inode(fsc->sb->s_root), >> - QUOTA_GET_MAX_BYTES, true); >> + get_quota_realm(mdsc, d_inode(fsc->sb->s_root), >> + QUOTA_GET_MAX_BYTES, &realm, true); > > While here you ignored it. > > I think we can just ignore it and the 'get_quota_realm()' will help initialize it. Yes, I would remove the initialization in ceph_quota_is_same_realm() then repost. > > Thanks > > - Xiubo > >> up_read(&mdsc->snap_rwsem); >> if (!realm) >> return false; >
diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c index 9d36c3532de1..e85a85b34a83 100644 --- a/fs/ceph/quota.c +++ b/fs/ceph/quota.c @@ -197,10 +197,10 @@ void ceph_cleanup_quotarealms_inodes(struct ceph_mds_client *mdsc) } /* - * This function walks through the snaprealm for an inode and returns the - * ceph_snap_realm for the first snaprealm that has quotas set (max_files, + * This function walks through the snaprealm for an inode and set the + * realmp with the first snaprealm that has quotas set (max_files, * max_bytes, or any, depending on the 'which_quota' argument). If the root is - * reached, return the root ceph_snap_realm instead. + * reached, set the realmp with the root ceph_snap_realm instead. * * Note that the caller is responsible for calling ceph_put_snap_realm() on the * returned realm. @@ -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_realm, 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); @@ -492,8 +497,8 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf) bool is_updated = false; down_read(&mdsc->snap_rwsem); - realm = get_quota_realm(mdsc, d_inode(fsc->sb->s_root), - QUOTA_GET_MAX_BYTES, true); + get_quota_realm(mdsc, d_inode(fsc->sb->s_root), + QUOTA_GET_MAX_BYTES, &realm, true); up_read(&mdsc->snap_rwsem); if (!realm) return false;
This issue is reported by smatch that get_quota_realm() might return ERR_PTR but we did not handle it. It's not a immediate bug, while we still should address it to avoid potential bugs if get_quota_realm() is changed to return other ERR_PTR in future. Set ceph_snap_realm's pointer in get_quota_realm()'s to address this issue, the pointer would be set to NULL if get_quota_realm() failed to get struct ceph_snap_realm, so no ERR_PTR would happen any more. Signed-off-by: Wenchao Hao <haowenchao2@huawei.com> --- V2: - Fix all potential invalid pointer access caused by get_quota_realm - Update commit comment and point it's not a immediate bug fs/ceph/quota.c | 41 +++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 18 deletions(-)