| Message ID | 20231123015340.3935321-1-haowenchao2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ceph: quota: Fix invalid pointer access if get_quota_realm return ERR_PTR | expand |
On 11/23/23 09:53, 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> > --- > V3: > - Remove redundant variable initialization in ceph_quota_is_same_realm > > 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 | 39 ++++++++++++++++++++++----------------- > 1 file changed, 22 insertions(+), 17 deletions(-) > > diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c > index 9d36c3532de1..b25906a5bfbb 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,7 +273,7 @@ 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) > @@ -277,6 +281,7 @@ 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; > 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); Some extra white spaces here. Otherwise LGTM. I will adjust it and applying it to the testing branch. 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..b25906a5bfbb 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,7 +273,7 @@ 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) @@ -277,6 +281,7 @@ 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; 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> --- V3: - Remove redundant variable initialization in ceph_quota_is_same_realm 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 | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-)