On Wed, Jan 20, 2021 at 1:43 PM Dan Carpenter <[email protected]> wrote:
>
> On Wed, Jan 20, 2021 at 12:01:59PM +0100, Ilya Dryomov wrote:
> > On Tue, Jan 19, 2021 at 8:46 PM Dan Carpenter <[email protected]>
> > wrote:
> > >
> > > tree:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > master
> > > head: 1e2a199f6ccdc15cf111d68d212e2fd4ce65682e
> > > commit: 2f713615ddd9d805b6c5e79c52e0e11af99d2bf1 libceph: move msgr1
> > > protocol implementation to its own file
> > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > >
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <[email protected]>
> > >
> > >
> > > cppcheck possible warnings: (new ones prefixed by >>, may not real
> > > problems)
> > >
> > > >> net/ceph/messenger_v1.c:1099:23: warning: Boolean result is used in
> > > >> bitwise operation. Clarify expression with parentheses.
> > > >> [clarifyCondition]
> > > BUG_ON(!con->in_msg ^ skip);
> > > ^
> > >
> > > vim +1099 net/ceph/messenger_v1.c
> > >
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1033 static int
> > > read_partial_message(struct ceph_connection *con)
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1034 {
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1035 struct ceph_msg
> > > *m = con->in_msg;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1036 int size;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1037 int end;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1038 int ret;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1039 unsigned int
> > > front_len, middle_len, data_len;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1040 bool do_datacrc =
> > > !ceph_test_opt(from_msgr(con->msgr), NOCRC);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1041 bool need_sign =
> > > (con->peer_features & CEPH_FEATURE_MSG_AUTH);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1042 u64 seq;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1043 u32 crc;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1044
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1045
> > > dout("read_partial_message con %p msg %p\n", con, m);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1046
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1047 /* header */
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1048 size = sizeof
> > > (con->in_hdr);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1049 end = size;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1050 ret =
> > > read_partial(con, end, size, &con->in_hdr);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1051 if (ret <= 0)
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1052 return
> > > ret;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1053
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1054 crc = crc32c(0,
> > > &con->in_hdr, offsetof(struct ceph_msg_header, crc));
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1055 if
> > > (cpu_to_le32(crc) != con->in_hdr.crc) {
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1056
> > > pr_err("read_partial_message bad hdr crc %u != expected %u\n",
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1057
> > > crc, con->in_hdr.crc);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1058 return
> > > -EBADMSG;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1059 }
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1060
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1061 front_len =
> > > le32_to_cpu(con->in_hdr.front_len);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1062 if (front_len >
> > > CEPH_MSG_MAX_FRONT_LEN)
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1063 return
> > > -EIO;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1064 middle_len =
> > > le32_to_cpu(con->in_hdr.middle_len);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1065 if (middle_len >
> > > CEPH_MSG_MAX_MIDDLE_LEN)
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1066 return
> > > -EIO;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1067 data_len =
> > > le32_to_cpu(con->in_hdr.data_len);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1068 if (data_len >
> > > CEPH_MSG_MAX_DATA_LEN)
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1069 return
> > > -EIO;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1070
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1071 /* verify seq# */
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1072 seq =
> > > le64_to_cpu(con->in_hdr.seq);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1073 if ((s64)seq -
> > > (s64)con->in_seq < 1) {
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1074
> > > pr_info("skipping %s%lld %s seq %lld expected %lld\n",
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1075
> > > ENTITY_NAME(con->peer_name),
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1076
> > > ceph_pr_addr(&con->peer_addr),
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1077
> > > seq, con->in_seq + 1);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1078
> > > con->in_base_pos = -front_len - middle_len - data_len -
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1079
> > > sizeof_footer(con);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1080
> > > con->in_tag = CEPH_MSGR_TAG_READY;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1081 return 1;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1082 } else if
> > > ((s64)seq - (s64)con->in_seq > 1) {
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1083
> > > pr_err("read_partial_message bad seq %lld expected %lld\n",
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1084
> > > seq, con->in_seq + 1);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1085
> > > con->error_msg = "bad message sequence # for incoming message";
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1086 return
> > > -EBADE;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1087 }
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1088
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1089 /* allocate
> > > message? */
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1090 if (!con->in_msg)
> > > {
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1091 int skip
> > > = 0;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1092
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1093 dout("got
> > > hdr type %d front %d data %d\n", con->in_hdr.type,
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1094
> > > front_len, data_len);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1095 ret =
> > > ceph_con_in_msg_alloc(con, &con->in_hdr, &skip);
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1096 if (ret <
> > > 0)
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1097
> > > return ret;
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 1098
> > > 2f713615ddd9d805 Ilya Dryomov 2020-11-12 @1099
> > > BUG_ON(!con->in_msg ^ skip);
> > >
> > > ! has higher precedence than ^. It's not clear that was intended
> > > necessarily.
> >
> > Hi Dan,
> >
> > This line and the surrounding code date back to 2013, commit
> > 2f713615ddd9 just moved it. It is correct (we either get a message
> > to work with or get instructed to skip, in the latter case con->in_msg
> > is expected to be NULL), so I'm inclined to leave it as is.
>
> You could silence the warning and make the code look more intentional
> by writing it like this:
>
> BUG_ON((!con->in_msg) ^ skip);
OK, I'll do that.
Thanks,
Ilya
