* [PATCH 0/2] ceph: misc fixes @ 2022-03-02 8:54 xiubli 2022-03-02 8:54 ` [PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir() xiubli 2022-03-02 8:54 ` [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps() xiubli 0 siblings, 2 replies; 6+ messages in thread From: xiubli @ 2022-03-02 8:54 UTC (permalink / raw) To: jlayton; +Cc: idryomov, vshankar, ceph-devel, Xiubo Li From: Xiubo Li <xiubli@redhat.com> Based the testing branch. Xiubo Li (2): ceph: fix inode reference leakage in ceph_get_snapdir() ceph: fix a NULL pointer dereference in ceph_handle_caps() fs/ceph/caps.c | 2 +- fs/ceph/inode.c | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir() 2022-03-02 8:54 [PATCH 0/2] ceph: misc fixes xiubli @ 2022-03-02 8:54 ` xiubli 2022-03-02 13:39 ` Jeff Layton 2022-03-02 8:54 ` [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps() xiubli 1 sibling, 1 reply; 6+ messages in thread From: xiubli @ 2022-03-02 8:54 UTC (permalink / raw) To: jlayton; +Cc: idryomov, vshankar, ceph-devel, Xiubo Li From: Xiubo Li <xiubli@redhat.com> The ceph_get_inode() will search for or insert a new inode into the hash for the given vino, and return a reference to it. If new is non-NULL, its reference is consumed. We should release the reference when in error handing cases. Signed-off-by: Xiubo Li <xiubli@redhat.com> --- fs/ceph/inode.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 8b0832271fdf..cbeba8a93a07 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -164,13 +164,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) if (!S_ISDIR(parent->i_mode)) { pr_warn_once("bad snapdir parent type (mode=0%o)\n", parent->i_mode); - return ERR_PTR(-ENOTDIR); + goto err; } if (!(inode->i_state & I_NEW) && !S_ISDIR(inode->i_mode)) { pr_warn_once("bad snapdir inode type (mode=0%o)\n", inode->i_mode); - return ERR_PTR(-ENOTDIR); + goto err; } inode->i_mode = parent->i_mode; @@ -190,6 +190,12 @@ struct inode *ceph_get_snapdir(struct inode *parent) } return inode; +err: + if ((inode->i_state & I_NEW)) + discard_new_inode(inode); + else + iput(inode); + return ERR_PTR(-ENOTDIR); } const struct inode_operations ceph_file_iops = { -- 2.27.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir() 2022-03-02 8:54 ` [PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir() xiubli @ 2022-03-02 13:39 ` Jeff Layton 0 siblings, 0 replies; 6+ messages in thread From: Jeff Layton @ 2022-03-02 13:39 UTC (permalink / raw) To: xiubli; +Cc: idryomov, vshankar, ceph-devel On Wed, 2022-03-02 at 16:54 +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > The ceph_get_inode() will search for or insert a new inode into the > hash for the given vino, and return a reference to it. If new is > non-NULL, its reference is consumed. > > We should release the reference when in error handing cases. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/inode.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 8b0832271fdf..cbeba8a93a07 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -164,13 +164,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) > if (!S_ISDIR(parent->i_mode)) { > pr_warn_once("bad snapdir parent type (mode=0%o)\n", > parent->i_mode); > - return ERR_PTR(-ENOTDIR); > + goto err; > } > > if (!(inode->i_state & I_NEW) && !S_ISDIR(inode->i_mode)) { > pr_warn_once("bad snapdir inode type (mode=0%o)\n", > inode->i_mode); > - return ERR_PTR(-ENOTDIR); > + goto err; > } > > inode->i_mode = parent->i_mode; > @@ -190,6 +190,12 @@ struct inode *ceph_get_snapdir(struct inode *parent) > } > > return inode; > +err: > + if ((inode->i_state & I_NEW)) > + discard_new_inode(inode); > + else > + iput(inode); > + return ERR_PTR(-ENOTDIR); > } > > const struct inode_operations ceph_file_iops = { Good catch! Reviewed-by: Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps() 2022-03-02 8:54 [PATCH 0/2] ceph: misc fixes xiubli 2022-03-02 8:54 ` [PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir() xiubli @ 2022-03-02 8:54 ` xiubli 2022-03-02 14:06 ` Jeff Layton 1 sibling, 1 reply; 6+ messages in thread From: xiubli @ 2022-03-02 8:54 UTC (permalink / raw) To: jlayton; +Cc: idryomov, vshankar, ceph-devel, Xiubo Li From: Xiubo Li <xiubli@redhat.com> The ceph_find_inode() may will fail and return NULL. Signed-off-by: Xiubo Li <xiubli@redhat.com> --- fs/ceph/caps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 0b36020207fd..0762b55fdbcb 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -4303,7 +4303,6 @@ void ceph_handle_caps(struct ceph_mds_session *session, /* lookup ino */ inode = ceph_find_inode(mdsc->fsc->sb, vino); - ci = ceph_inode(inode); dout(" op %s ino %llx.%llx inode %p\n", ceph_cap_op_name(op), vino.ino, vino.snap, inode); @@ -4333,6 +4332,7 @@ void ceph_handle_caps(struct ceph_mds_session *session, } goto flush_cap_releases; } + ci = ceph_inode(inode); /* these will work even if we don't have a cap yet */ switch (op) { -- 2.27.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps() 2022-03-02 8:54 ` [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps() xiubli @ 2022-03-02 14:06 ` Jeff Layton 2022-03-03 1:08 ` Xiubo Li 0 siblings, 1 reply; 6+ messages in thread From: Jeff Layton @ 2022-03-02 14:06 UTC (permalink / raw) To: xiubli; +Cc: idryomov, vshankar, ceph-devel On Wed, 2022-03-02 at 16:54 +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > The ceph_find_inode() may will fail and return NULL. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 0b36020207fd..0762b55fdbcb 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -4303,7 +4303,6 @@ void ceph_handle_caps(struct ceph_mds_session *session, > > /* lookup ino */ > inode = ceph_find_inode(mdsc->fsc->sb, vino); > - ci = ceph_inode(inode); > dout(" op %s ino %llx.%llx inode %p\n", ceph_cap_op_name(op), vino.ino, > vino.snap, inode); > > @@ -4333,6 +4332,7 @@ void ceph_handle_caps(struct ceph_mds_session *session, > } > goto flush_cap_releases; > } > + ci = ceph_inode(inode); > > /* these will work even if we don't have a cap yet */ > switch (op) { I don't think this is an actual bug. We're just assigning "ci" here and that doesn't involve a dereference of inode. If "inode" is NULL, then ci will be close to NULL, but it doesn't get used in that case. Assigning this lower in the function is fine though, and it discourages anyone trying to use ci when they shouldn't, so you can add my ack, but maybe fix the patch description since there is no dereference here. Acked-by: Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps() 2022-03-02 14:06 ` Jeff Layton @ 2022-03-03 1:08 ` Xiubo Li 0 siblings, 0 replies; 6+ messages in thread From: Xiubo Li @ 2022-03-03 1:08 UTC (permalink / raw) To: Jeff Layton; +Cc: idryomov, vshankar, ceph-devel On 3/2/22 10:06 PM, Jeff Layton wrote: > On Wed, 2022-03-02 at 16:54 +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> The ceph_find_inode() may will fail and return NULL. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/caps.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 0b36020207fd..0762b55fdbcb 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -4303,7 +4303,6 @@ void ceph_handle_caps(struct ceph_mds_session *session, >> >> /* lookup ino */ >> inode = ceph_find_inode(mdsc->fsc->sb, vino); >> - ci = ceph_inode(inode); >> dout(" op %s ino %llx.%llx inode %p\n", ceph_cap_op_name(op), vino.ino, >> vino.snap, inode); >> >> @@ -4333,6 +4332,7 @@ void ceph_handle_caps(struct ceph_mds_session *session, >> } >> goto flush_cap_releases; >> } >> + ci = ceph_inode(inode); >> >> /* these will work even if we don't have a cap yet */ >> switch (op) { > I don't think this is an actual bug. We're just assigning "ci" here and > that doesn't involve a dereference of inode. If "inode" is NULL, then ci > will be close to NULL, but it doesn't get used in that case. Right, I misread the code. > Assigning this lower in the function is fine though, and it discourages > anyone trying to use ci when they shouldn't, so you can add my ack, but > maybe fix the patch description since there is no dereference here. > > Acked-by: Jeff Layton <jlayton@kernel.org> Sure, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-03 1:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-02 8:54 [PATCH 0/2] ceph: misc fixes xiubli 2022-03-02 8:54 ` [PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir() xiubli 2022-03-02 13:39 ` Jeff Layton 2022-03-02 8:54 ` [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps() xiubli 2022-03-02 14:06 ` Jeff Layton 2022-03-03 1:08 ` Xiubo Li
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.