Linux kernel mirror (for testing)
git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel
os
linux
Merge branch 'net-netconsole-refactoring-and-warning-fix'
Breno Leitao says:
====================
net: netconsole refactoring and warning fix
The netconsole driver was showing a warning related to userdata
information, depending on the message size being transmitted:
------------[ cut here ]------------
WARNING: CPU: 13 PID: 3013042 at drivers/net/netconsole.c:1122 write_ext_msg+0x3b6/0x3d0
? write_ext_msg+0x3b6/0x3d0
console_flush_all+0x1e9/0x330
...
Identifying the cause of this warning proved to be non-trivial due to:
* The write_ext_msg() function being over 100 lines long
* Extensive use of pointer arithmetic
* Inconsistent naming conventions and concept application
The send_ext_msg() function grew organically over time:
* Initially, the UDP packet consisted of a header and body
* Later additions included release prepend and userdata
* Naming became inconsistent (e.g., "body" excludes userdata, "header"
excludes prepended release)
This lack of consistency made investigating issues like the above warning
more challenging than what it should be.
To address these issues, the following steps were taken:
* Breaking down write_ext_msg() into smaller functions with clear scopes
* Improving readability and reasoning about the code
* Simplifying and clarifying naming conventions
Warning Fix
-----------
The warning occurred when there was insufficient buffer space to append
userdata. While this scenario is acceptable (as userdata can be sent in a
separate packet later), the kernel was incorrectly raising a warning. A
one-line fix has been implemented to resolve this issue.
The fix was already sent to net, and is already available in net-next
also.
v4:
* https://lore.kernel.org/all/20240930131214.3771313-1-leitao@debian.org/
v3:
* https://lore.kernel.org/all/20240910100410.2690012-1-leitao@debian.org/
v2:
* https://lore.kernel.org/all/20240909130756.2722126-1-leitao@debian.org/
v1:
* https://lore.kernel.org/all/20240903140757.2802765-1-leitao@debian.org/
====================
Link: https://patch.msgid.link/20241017095028.3131508-1-leitao@debian.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
+138
-71
+138
-71
drivers/net/netconsole.c
+138
-71
drivers/net/netconsole.c
···
1058
1058
.notifier_call = netconsole_netdev_event,
1059
1059
};
1060
1060
1061
-
/**
1062
-
* send_ext_msg_udp - send extended log message to target
1063
-
* @nt: target to send message to
1064
-
* @msg: extended log message to send
1065
-
* @msg_len: length of message
1066
-
*
1067
-
* Transfer extended log @msg to @nt. If @msg is longer than
1068
-
* MAX_PRINT_CHUNK, it'll be split and transmitted in multiple chunks with
1069
-
* ncfrag header field added to identify them.
1070
-
*/
1071
-
static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
1072
-
int msg_len)
1061
+
static void send_msg_no_fragmentation(struct netconsole_target *nt,
1062
+
const char *msg,
1063
+
int msg_len,
1064
+
int release_len)
1073
1065
{
1074
1066
static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
1075
-
const char *header, *body;
1076
-
int offset = 0;
1077
-
int header_len, body_len;
1078
-
const char *msg_ready = msg;
1067
+
const char *userdata = NULL;
1079
1068
const char *release;
1080
-
int release_len = 0;
1069
+
1070
+
#ifdef CONFIG_NETCONSOLE_DYNAMIC
1071
+
userdata = nt->userdata_complete;
1072
+
#endif
1073
+
1074
+
if (release_len) {
1075
+
release = init_utsname()->release;
1076
+
1077
+
scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
1078
+
msg_len += release_len;
1079
+
} else {
1080
+
memcpy(buf, msg, msg_len);
1081
+
}
1082
+
1083
+
if (userdata)
1084
+
msg_len += scnprintf(&buf[msg_len],
1085
+
MAX_PRINT_CHUNK - msg_len,
1086
+
"%s", userdata);
1087
+
1088
+
netpoll_send_udp(&nt->np, buf, msg_len);
1089
+
}
1090
+
1091
+
static void append_release(char *buf)
1092
+
{
1093
+
const char *release;
1094
+
1095
+
release = init_utsname()->release;
1096
+
scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
1097
+
}
1098
+
1099
+
static void send_fragmented_body(struct netconsole_target *nt, char *buf,
1100
+
const char *msgbody, int header_len,
1101
+
int msgbody_len)
1102
+
{
1103
+
const char *userdata = NULL;
1104
+
int body_len, offset = 0;
1081
1105
int userdata_len = 0;
1082
-
char *userdata = NULL;
1083
1106
1084
1107
#ifdef CONFIG_NETCONSOLE_DYNAMIC
1085
1108
userdata = nt->userdata_complete;
1086
1109
userdata_len = nt->userdata_length;
1087
1110
#endif
1088
1111
1089
-
if (nt->release) {
1090
-
release = init_utsname()->release;
1091
-
release_len = strlen(release) + 1;
1092
-
}
1093
-
1094
-
if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK) {
1095
-
/* No fragmentation needed */
1096
-
if (nt->release) {
1097
-
scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
1098
-
msg_len += release_len;
1099
-
} else {
1100
-
memcpy(buf, msg, msg_len);
1101
-
}
1102
-
1103
-
if (userdata)
1104
-
msg_len += scnprintf(&buf[msg_len],
1105
-
MAX_PRINT_CHUNK - msg_len,
1106
-
"%s", userdata);
1107
-
1108
-
msg_ready = buf;
1109
-
netpoll_send_udp(&nt->np, msg_ready, msg_len);
1110
-
return;
1111
-
}
1112
-
1113
-
/* need to insert extra header fields, detect header and body */
1114
-
header = msg;
1115
-
body = memchr(msg, ';', msg_len);
1116
-
if (WARN_ON_ONCE(!body))
1117
-
return;
1118
-
1119
-
header_len = body - header;
1120
-
body_len = msg_len - header_len - 1;
1121
-
body++;
1122
-
1123
-
/*
1124
-
* Transfer multiple chunks with the following extra header.
1125
-
* "ncfrag=<byte-offset>/<total-bytes>"
1112
+
/* body_len represents the number of bytes that will be sent. This is
1113
+
* bigger than MAX_PRINT_CHUNK, thus, it will be split in multiple
1114
+
* packets
1126
1115
*/
1127
-
if (nt->release)
1128
-
scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
1129
-
memcpy(buf + release_len, header, header_len);
1130
-
header_len += release_len;
1116
+
body_len = msgbody_len + userdata_len;
1131
1117
1132
-
while (offset < body_len + userdata_len) {
1118
+
/* In each iteration of the while loop below, we send a packet
1119
+
* containing the header and a portion of the body. The body is
1120
+
* composed of two parts: msgbody and userdata. We keep track of how
1121
+
* many bytes have been sent so far using the offset variable, which
1122
+
* ranges from 0 to the total length of the body.
1123
+
*/
1124
+
while (offset < body_len) {
1133
1125
int this_header = header_len;
1126
+
bool msgbody_written = false;
1134
1127
int this_offset = 0;
1135
1128
int this_chunk = 0;
1136
1129
1137
1130
this_header += scnprintf(buf + this_header,
1138
-
sizeof(buf) - this_header,
1131
+
MAX_PRINT_CHUNK - this_header,
1139
1132
",ncfrag=%d/%d;", offset,
1140
-
body_len + userdata_len);
1133
+
body_len);
1141
1134
1142
-
/* Not all body data has been written yet */
1143
-
if (offset < body_len) {
1144
-
this_chunk = min(body_len - offset,
1135
+
/* Not all msgbody data has been written yet */
1136
+
if (offset < msgbody_len) {
1137
+
this_chunk = min(msgbody_len - offset,
1145
1138
MAX_PRINT_CHUNK - this_header);
1146
1139
if (WARN_ON_ONCE(this_chunk <= 0))
1147
1140
return;
1148
-
memcpy(buf + this_header, body + offset, this_chunk);
1141
+
memcpy(buf + this_header, msgbody + offset, this_chunk);
1149
1142
this_offset += this_chunk;
1150
1143
}
1151
-
/* Body is fully written and there is pending userdata to write,
1152
-
* append userdata in this chunk
1144
+
1145
+
/* msgbody was finally written, either in the previous
1146
+
* messages and/or in the current buf. Time to write
1147
+
* the userdata.
1153
1148
*/
1154
-
if (offset + this_offset >= body_len &&
1155
-
offset + this_offset < userdata_len + body_len) {
1156
-
int sent_userdata = (offset + this_offset) - body_len;
1149
+
msgbody_written |= offset + this_offset >= msgbody_len;
1150
+
1151
+
/* Msg body is fully written and there is pending userdata to
1152
+
* write, append userdata in this chunk
1153
+
*/
1154
+
if (msgbody_written && offset + this_offset < body_len) {
1155
+
/* Track how much user data was already sent. First
1156
+
* time here, sent_userdata is zero
1157
+
*/
1158
+
int sent_userdata = (offset + this_offset) - msgbody_len;
1159
+
/* offset of bytes used in current buf */
1157
1160
int preceding_bytes = this_chunk + this_header;
1158
1161
1159
1162
if (WARN_ON_ONCE(sent_userdata < 0))
···
1181
1178
netpoll_send_udp(&nt->np, buf, this_header + this_offset);
1182
1179
offset += this_offset;
1183
1180
}
1181
+
}
1182
+
1183
+
static void send_msg_fragmented(struct netconsole_target *nt,
1184
+
const char *msg,
1185
+
int msg_len,
1186
+
int release_len)
1187
+
{
1188
+
static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
1189
+
int header_len, msgbody_len;
1190
+
const char *msgbody;
1191
+
1192
+
/* need to insert extra header fields, detect header and msgbody */
1193
+
msgbody = memchr(msg, ';', msg_len);
1194
+
if (WARN_ON_ONCE(!msgbody))
1195
+
return;
1196
+
1197
+
header_len = msgbody - msg;
1198
+
msgbody_len = msg_len - header_len - 1;
1199
+
msgbody++;
1200
+
1201
+
/*
1202
+
* Transfer multiple chunks with the following extra header.
1203
+
* "ncfrag=<byte-offset>/<total-bytes>"
1204
+
*/
1205
+
if (release_len)
1206
+
append_release(buf);
1207
+
1208
+
/* Copy the header into the buffer */
1209
+
memcpy(buf + release_len, msg, header_len);
1210
+
header_len += release_len;
1211
+
1212
+
/* for now on, the header will be persisted, and the msgbody
1213
+
* will be replaced
1214
+
*/
1215
+
send_fragmented_body(nt, buf, msgbody, header_len, msgbody_len);
1216
+
}
1217
+
1218
+
/**
1219
+
* send_ext_msg_udp - send extended log message to target
1220
+
* @nt: target to send message to
1221
+
* @msg: extended log message to send
1222
+
* @msg_len: length of message
1223
+
*
1224
+
* Transfer extended log @msg to @nt. If @msg is longer than
1225
+
* MAX_PRINT_CHUNK, it'll be split and transmitted in multiple chunks with
1226
+
* ncfrag header field added to identify them.
1227
+
*/
1228
+
static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
1229
+
int msg_len)
1230
+
{
1231
+
int userdata_len = 0;
1232
+
int release_len = 0;
1233
+
1234
+
#ifdef CONFIG_NETCONSOLE_DYNAMIC
1235
+
userdata_len = nt->userdata_length;
1236
+
#endif
1237
+
1238
+
if (nt->release)
1239
+
release_len = strlen(init_utsname()->release) + 1;
1240
+
1241
+
if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK)
1242
+
return send_msg_no_fragmentation(nt, msg, msg_len, release_len);
1243
+
1244
+
return send_msg_fragmented(nt, msg, msg_len, release_len);
1184
1245
}
1185
1246
1186
1247
static void write_ext_msg(struct console *con, const char *msg,