Re: [PATCH v3 3/4] ceph: switch to WARN_ON and bubble up errnos to the callers

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 2020/6/24 2:02, Jeff Layton wrote:
> On Mon, 2020-06-22 at 09:24 -0400, xiubli@xxxxxxxxxx wrote:
> > From: Xiubo Li <xiubli@xxxxxxxxxx>
> >
> > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > ---
> >   fs/ceph/mds_client.c | 46 +++++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index f996363..f29cb11 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -1168,7 +1168,7 @@ static struct ceph_msg *create_session_msg(u32 op, u64 seq)
> >   
> >   static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
> >   #define FEATURE_BYTES(c) (DIV_ROUND_UP((size_t)feature_bits[c - 1] + 1, 64) * 8)
> > -static void encode_supported_features(void **p, void *end)
> > +static int encode_supported_features(void **p, void *end)
> >   {
> >   	static const size_t count = ARRAY_SIZE(feature_bits);
> >   
> > @@ -1176,16 +1176,22 @@ static void encode_supported_features(void **p, void *end)
> >   		size_t i;
> >   		size_t size = FEATURE_BYTES(count);
> >   
> > -		BUG_ON(*p + 4 + size > end);
> > +		if (WARN_ON(*p + 4 + size > end))
> > +			return -ERANGE;
> > +
> Nice cleanup.
>
> Let's use WARN_ON_ONCE instead?
>
> It's better not to spam the logs if this is happening all over the
> place. Also, I'm not sure that ERANGE is the right error here, but I
> can't think of anything better. At least it should be distinctive.

Yeah, it  makes sense and I will fix it.

Thanks.


> >   		ceph_encode_32(p, size);
> >   		memset(*p, 0, size);
> >   		for (i = 0; i < count; i++)
> >   			((unsigned char*)(*p))[i / 8] |= BIT(feature_bits[i] % 8);
> >   		*p += size;
> >   	} else {
> > -		BUG_ON(*p + 4 > end);
> > +		if (WARN_ON(*p + 4 > end))
> > +			return -ERANGE;
> > +
> >   		ceph_encode_32(p, 0);
> >   	}
> > +
> > +	return 0;
> >   }
> >   
> >   /*
> > @@ -1203,6 +1209,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
> >   	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
> >   	size_t size, count;
> >   	void *p, *end;
> > +	int ret;
> >   
> >   	const char* metadata[][2] = {
> >   		{"hostname", mdsc->nodename},
> > @@ -1232,7 +1239,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
> >   			   GFP_NOFS, false);
> >   	if (!msg) {
> >   		pr_err("create_session_msg ENOMEM creating msg\n");
> > -		return NULL;
> > +		return ERR_PTR(-ENOMEM);
> >   	}
> >   	p = msg->front.iov_base;
> >   	end = p + msg->front.iov_len;
> > @@ -1269,7 +1276,13 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
> >   		p += val_len;
> >   	}
> >   
> > -	encode_supported_features(&p, end);
> > +	ret = encode_supported_features(&p, end);
> > +	if (ret) {
> > +		pr_err("encode_supported_features failed!\n");
> > +		ceph_msg_put(msg);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> >   	msg->front.iov_len = p - msg->front.iov_base;
> >   	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
> >   
> > @@ -1297,8 +1310,8 @@ static int __open_session(struct ceph_mds_client *mdsc,
> >   
> >   	/* send connect message */
> >   	msg = create_session_open_msg(mdsc, session->s_seq);
> > -	if (!msg)
> > -		return -ENOMEM;
> > +	if (IS_ERR(msg))
> > +		return PTR_ERR(msg);
> >   	ceph_con_send(&session->s_con, msg);
> >   	return 0;
> >   }
> > @@ -1312,6 +1325,7 @@ static int __open_session(struct ceph_mds_client *mdsc,
> >   __open_export_target_session(struct ceph_mds_client *mdsc, int target)
> >   {
> >   	struct ceph_mds_session *session;
> > +	int ret;
> >   
> >   	session = __ceph_lookup_mds_session(mdsc, target);
> >   	if (!session) {
> > @@ -1320,8 +1334,11 @@ static int __open_session(struct ceph_mds_client *mdsc,
> >   			return session;
> >   	}
> >   	if (session->s_state == CEPH_MDS_SESSION_NEW ||
> > -	    session->s_state == CEPH_MDS_SESSION_CLOSING)
> > -		__open_session(mdsc, session);
> > +	    session->s_state == CEPH_MDS_SESSION_CLOSING) {
> > +		ret = __open_session(mdsc, session);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	}
> >   
> >   	return session;
> >   }
> > @@ -2520,7 +2537,12 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
> >   		ceph_encode_copy(&p, &ts, sizeof(ts));
> >   	}
> >   
> > -	BUG_ON(p > end);
> > +	if (WARN_ON(p > end)) {
> > +		ceph_msg_put(msg);
> > +		msg = ERR_PTR(-ERANGE);
> > +		goto out_free2;
> > +	}
> > +
> >   	msg->front.iov_len = p - msg->front.iov_base;
> >   	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
> >   
> > @@ -2756,7 +2778,9 @@ static void __do_request(struct ceph_mds_client *mdsc,
> >   		}
> >   		if (session->s_state == CEPH_MDS_SESSION_NEW ||
> >   		    session->s_state == CEPH_MDS_SESSION_CLOSING) {
> > -			__open_session(mdsc, session);
> > +			err = __open_session(mdsc, session);
> > +			if (err)
> > +				goto out_session;
> >   			/* retry the same mds later */
> >   			if (random)
> >   				req->r_resend_mds = mds;