The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF
leads to much larger kernel stack usage, as seen from the warnings
about functions that now exceed the 2048 byte limit:
drivers/media/i2c/tvp5150.c:253:1: error: the frame size of 3936 bytes is larger than 2048 bytes
drivers/media/tuners/r820t.c:1327:1: error: the frame size of 2816 bytes is larger than 2048 bytes
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:16552:1: error: the frame size of 3144 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
fs/ocfs2/aops.c:1892:1: error: the frame size of 2088 bytes is larger than 2048 bytes
fs/ocfs2/dlm/dlmrecovery.c:737:1: error: the frame size of 2088 bytes is larger than 2048 bytes
fs/ocfs2/namei.c:1677:1: error: the frame size of 2584 bytes is larger than 2048 bytes
fs/ocfs2/super.c:1186:1: error: the frame size of 2640 bytes is larger than 2048 bytes
fs/ocfs2/xattr.c:3678:1: error: the frame size of 2176 bytes is larger than 2048 bytes
net/bluetooth/l2cap_core.c:7056:1: error: the frame size of 2144 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
net/bluetooth/l2cap_core.c: In function 'l2cap_recv_frame':
net/bridge/br_netlink.c:1505:1: error: the frame size of 2448 bytes is larger than 2048 bytes
net/ieee802154/nl802154.c:548:1: error: the frame size of 2232 bytes is larger than 2048 bytes
net/wireless/nl80211.c:1726:1: error: the frame size of 2224 bytes is larger than 2048 bytes
net/wireless/nl80211.c:2357:1: error: the frame size of 4584 bytes is larger than 2048 bytes
net/wireless/nl80211.c:5108:1: error: the frame size of 2760 bytes is larger than 2048 bytes
net/wireless/nl80211.c:6472:1: error: the frame size of 2112 bytes is larger than 2048 bytes
The structleak plugin was previously disabled for CONFIG_COMPILE_TEST,
but meant we missed some bugs, so this time we should address them.
The frame size warnings are distracting, and risking a kernel stack
overflow is generally not beneficial to performance, so it may be best
to disallow that particular combination. This can be done by turning
off either one. I picked the dependency in GCC_PLUGIN_STRUCTLEAK_BYREF
and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, as this option is designed to
make uninitialized stack usage less harmful when enabled on its own,
but it also prevents KASAN from detecting those cases in which it was
in fact needed.
KASAN_STACK is currently implied by KASAN on gcc, but could be made a
user selectable option if we want to allow combining (non-stack) KASAN
with GCC_PLUGIN_STRUCTLEAK_BYREF.
Note that it would be possible to specifically address the files that
print the warning, but presumably the overall stack usage is still
significantly higher than in other configurations, so this would not
address the full problem.
I could not test this with CONFIG_INIT_STACK_ALL, which may or may not
suffer from a similar problem.
Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
Signed-off-by: Arnd Bergmann <[email protected]>
---
[v2] do it for both GCC_PLUGIN_STRUCTLEAK_BYREF and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
---
security/Kconfig.hardening | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index a1ffe2eb4d5f..af4c979b38ee 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -61,6 +61,7 @@ choice
config GCC_PLUGIN_STRUCTLEAK_BYREF
bool "zero-init structs passed by reference (strong)"
depends on GCC_PLUGINS
+ depends on !(KASAN && KASAN_STACK=1)
select GCC_PLUGIN_STRUCTLEAK
help
Zero-initialize any structures on the stack that may
@@ -70,9 +71,15 @@ choice
exposures, like CVE-2017-1000410:
https://git.kernel.org/linus/06e7e776ca4d3654
+ As a side-effect, this keeps a lot of variables on the
+ stack that can otherwise be optimized out, so combining
+ this with CONFIG_KASAN_STACK can lead to a stack overflow
+ and is disallowed.
+
config GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
bool "zero-init anything passed by reference (very strong)"
depends on GCC_PLUGINS
+ depends on !(KASAN && KASAN_STACK=1)
select GCC_PLUGIN_STRUCTLEAK
help
Zero-initialize any stack variables that may be passed
--
2.20.0
An earlier patch I sent reduced the stack usage enough to get
below the warning limit, and I could show this was safe, but with
GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, it gets worse again because large stack
variables in the same function no longer overlap:
drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'translate_scan.isra.2':
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:322:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
Split out the largest two blocks in the affected function into two
separate functions and mark those noinline_for_stack.
Fixes: 8c5af16f7953 ("staging: rtl8712: reduce stack usage")
Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 157 ++++++++++--------
1 file changed, 88 insertions(+), 69 deletions(-)
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index a224797cd993..fdc1df99d852 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -124,10 +124,91 @@ static inline void handle_group_key(struct ieee_param *param,
}
}
-static noinline_for_stack char *translate_scan(struct _adapter *padapter,
- struct iw_request_info *info,
- struct wlan_network *pnetwork,
- char *start, char *stop)
+static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
+ struct wlan_network *pnetwork,
+ struct iw_event *iwe,
+ char *start, char *stop)
+{
+ /* parsing WPA/WPA2 IE */
+ u8 buf[MAX_WPA_IE_LEN];
+ u8 wpa_ie[255], rsn_ie[255];
+ u16 wpa_len = 0, rsn_len = 0;
+ int n, i;
+
+ r8712_get_sec_ie(pnetwork->network.IEs,
+ pnetwork->network.IELength, rsn_ie, &rsn_len,
+ wpa_ie, &wpa_len);
+ if (wpa_len > 0) {
+ memset(buf, 0, MAX_WPA_IE_LEN);
+ n = sprintf(buf, "wpa_ie=");
+ for (i = 0; i < wpa_len; i++) {
+ n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
+ "%02x", wpa_ie[i]);
+ if (n >= MAX_WPA_IE_LEN)
+ break;
+ }
+ memset(iwe, 0, sizeof(*iwe));
+ iwe->cmd = IWEVCUSTOM;
+ iwe->u.data.length = (u16)strlen(buf);
+ start = iwe_stream_add_point(info, start, stop,
+ iwe, buf);
+ memset(iwe, 0, sizeof(*iwe));
+ iwe->cmd = IWEVGENIE;
+ iwe->u.data.length = (u16)wpa_len;
+ start = iwe_stream_add_point(info, start, stop,
+ iwe, wpa_ie);
+ }
+ if (rsn_len > 0) {
+ memset(buf, 0, MAX_WPA_IE_LEN);
+ n = sprintf(buf, "rsn_ie=");
+ for (i = 0; i < rsn_len; i++) {
+ n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
+ "%02x", rsn_ie[i]);
+ if (n >= MAX_WPA_IE_LEN)
+ break;
+ }
+ memset(iwe, 0, sizeof(*iwe));
+ iwe->cmd = IWEVCUSTOM;
+ iwe->u.data.length = strlen(buf);
+ start = iwe_stream_add_point(info, start, stop,
+ iwe, buf);
+ memset(iwe, 0, sizeof(*iwe));
+ iwe->cmd = IWEVGENIE;
+ iwe->u.data.length = rsn_len;
+ start = iwe_stream_add_point(info, start, stop, iwe,
+ rsn_ie);
+ }
+
+ return start;
+}
+
+static noinline_for_stack char *translate_scan_wps(struct iw_request_info *info,
+ struct wlan_network *pnetwork,
+ struct iw_event *iwe,
+ char *start, char *stop)
+{
+ /* parsing WPS IE */
+ u8 wps_ie[512];
+ uint wps_ielen;
+
+ if (r8712_get_wps_ie(pnetwork->network.IEs,
+ pnetwork->network.IELength,
+ wps_ie, &wps_ielen)) {
+ if (wps_ielen > 2) {
+ iwe->cmd = IWEVGENIE;
+ iwe->u.data.length = (u16)wps_ielen;
+ start = iwe_stream_add_point(info, start, stop,
+ iwe, wps_ie);
+ }
+ }
+
+ return start;
+}
+
+static char *translate_scan(struct _adapter *padapter,
+ struct iw_request_info *info,
+ struct wlan_network *pnetwork,
+ char *start, char *stop)
{
struct iw_event iwe;
struct ieee80211_ht_cap *pht_capie;
@@ -240,73 +321,11 @@ static noinline_for_stack char *translate_scan(struct _adapter *padapter,
/* Check if we added any event */
if ((current_val - start) > iwe_stream_lcp_len(info))
start = current_val;
- /* parsing WPA/WPA2 IE */
- {
- u8 buf[MAX_WPA_IE_LEN];
- u8 wpa_ie[255], rsn_ie[255];
- u16 wpa_len = 0, rsn_len = 0;
- int n;
-
- r8712_get_sec_ie(pnetwork->network.IEs,
- pnetwork->network.IELength, rsn_ie, &rsn_len,
- wpa_ie, &wpa_len);
- if (wpa_len > 0) {
- memset(buf, 0, MAX_WPA_IE_LEN);
- n = sprintf(buf, "wpa_ie=");
- for (i = 0; i < wpa_len; i++) {
- n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
- "%02x", wpa_ie[i]);
- if (n >= MAX_WPA_IE_LEN)
- break;
- }
- memset(&iwe, 0, sizeof(iwe));
- iwe.cmd = IWEVCUSTOM;
- iwe.u.data.length = (u16)strlen(buf);
- start = iwe_stream_add_point(info, start, stop,
- &iwe, buf);
- memset(&iwe, 0, sizeof(iwe));
- iwe.cmd = IWEVGENIE;
- iwe.u.data.length = (u16)wpa_len;
- start = iwe_stream_add_point(info, start, stop,
- &iwe, wpa_ie);
- }
- if (rsn_len > 0) {
- memset(buf, 0, MAX_WPA_IE_LEN);
- n = sprintf(buf, "rsn_ie=");
- for (i = 0; i < rsn_len; i++) {
- n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
- "%02x", rsn_ie[i]);
- if (n >= MAX_WPA_IE_LEN)
- break;
- }
- memset(&iwe, 0, sizeof(iwe));
- iwe.cmd = IWEVCUSTOM;
- iwe.u.data.length = strlen(buf);
- start = iwe_stream_add_point(info, start, stop,
- &iwe, buf);
- memset(&iwe, 0, sizeof(iwe));
- iwe.cmd = IWEVGENIE;
- iwe.u.data.length = rsn_len;
- start = iwe_stream_add_point(info, start, stop, &iwe,
- rsn_ie);
- }
- }
- { /* parsing WPS IE */
- u8 wps_ie[512];
- uint wps_ielen;
+ start = translate_scan_wpa(info, pnetwork, &iwe, start, stop);
+
+ start = translate_scan_wps(info, pnetwork, &iwe, start, stop);
- if (r8712_get_wps_ie(pnetwork->network.IEs,
- pnetwork->network.IELength,
- wps_ie, &wps_ielen)) {
- if (wps_ielen > 2) {
- iwe.cmd = IWEVGENIE;
- iwe.u.data.length = (u16)wps_ielen;
- start = iwe_stream_add_point(info, start, stop,
- &iwe, wps_ie);
- }
- }
- }
/* Add quality statistics */
iwe.cmd = IWEVQUAL;
rssi = r8712_signal_scale_mapping(pnetwork->network.Rssi);
--
2.20.0
The lpfc_debug_dump_all_queues() function repeatedly calls into
lpfc_debug_dump_qe(), which has a temporary 128 byte buffer.
This was fine before the introduction of CONFIG_GCC_PLUGIN_STRUCTLEAK_VERBOSE
because each instance could occupy the same stack slot. However,
now they each get their own copy, which leads to a huge increase
in stack usage as seen from the compiler warning:
drivers/scsi/lpfc/lpfc_debugfs.c: In function 'lpfc_debug_dump_all_queues':
drivers/scsi/lpfc/lpfc_debugfs.c:6474:1: error: the frame size of 1712 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
Avoid this by not marking lpfc_debug_dump_qe() as inline so the
compiler can choose to emit a static version of this function
when it's needed or otherwise silently drop it. As an added benefit,
not inlining multiple copies of this function means we save several
kilobytes of .text section, reducing the file size from 47kb to 43.
It is somewhat unusual to have a function that is static but not
inline in a header file, but this does not cause problems here
because it is only used by other inline functions. It would
however seem reasonable to move all the lpfc_debug_dump_* functions
into lpfc_debugfs.c and not mark them inline as a later cleanup.
Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/scsi/lpfc/lpfc_debugfs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/lpfc/lpfc_debugfs.h b/drivers/scsi/lpfc/lpfc_debugfs.h
index 2322ddb085c0..34070874616d 100644
--- a/drivers/scsi/lpfc/lpfc_debugfs.h
+++ b/drivers/scsi/lpfc/lpfc_debugfs.h
@@ -330,7 +330,7 @@ enum {
* This function dumps an entry indexed by @idx from a queue specified by the
* queue descriptor @q.
**/
-static inline void
+static void
lpfc_debug_dump_qe(struct lpfc_queue *q, uint32_t idx)
{
char line_buf[LPFC_LBUF_SZ];
--
2.20.0
With the new CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL option, the stack
usage in the ipvs debug output grows because each instance of
IP_VS_DBG_BUF() now has its own buffer of 160 bytes that add up
rather than reusing the stack slots:
net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_sched_persist':
net/netfilter/ipvs/ip_vs_core.c:427:1: error: the frame size of 1052 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_new_conn_out':
net/netfilter/ipvs/ip_vs_core.c:1231:1: error: the frame size of 1048 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_out':
net/netfilter/ipvs/ip_vs_ftp.c:397:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_in':
net/netfilter/ipvs/ip_vs_ftp.c:555:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
Since printk() already has a way to print IPv4/IPv6 addresses using
the %pIS format string, use that instead, combined with a macro that
creates a local sockaddr structure on the stack. These will still
add up, but the stack frames are now under 200 bytes.
Signed-off-by: Arnd Bergmann <[email protected]>
---
I'm not sure this actually does what I think it does. Someone
needs to verify that we correctly print the addresses here.
I've also only added three files that caused the warning messages
to be reported. There are still a lot of other instances of
IP_VS_DBG_BUF() that could be converted the same way after the
basic idea is confirmed.
---
include/net/ip_vs.h | 71 +++++++++++++++++++--------------
net/netfilter/ipvs/ip_vs_core.c | 44 ++++++++++----------
net/netfilter/ipvs/ip_vs_ftp.c | 20 +++++-----
3 files changed, 72 insertions(+), 63 deletions(-)
diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index 3759167f91f5..3dfbeef67be6 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -227,6 +227,16 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
sizeof(ip_vs_dbg_buf), addr, \
&ip_vs_dbg_idx)
+#define IP_VS_DBG_SOCKADDR4(fam, addr, port) \
+ (struct sockaddr*)&(struct sockaddr_in) \
+ { .sin_family = (fam), .sin_addr = (addr)->in, .sin_port = (port) }
+#define IP_VS_DBG_SOCKADDR6(fam, addr, port) \
+ (struct sockaddr*)&(struct sockaddr_in6) \
+ { .sin6_family = (fam), .sin6_addr = (addr)->in6, .sin6_port = (port) }
+#define IP_VS_DBG_SOCKADDR(fam, addr, port) (fam == AF_INET ? \
+ IP_VS_DBG_SOCKADDR4(fam, addr, port) : \
+ IP_VS_DBG_SOCKADDR6(fam, addr, port))
+
#define IP_VS_DBG(level, msg, ...) \
do { \
if (level <= ip_vs_get_debug_level()) \
@@ -251,6 +261,7 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
#else /* NO DEBUGGING at ALL */
#define IP_VS_DBG_BUF(level, msg...) do {} while (0)
#define IP_VS_ERR_BUF(msg...) do {} while (0)
+#define IP_VS_DBG_SOCKADDR(fam, addr, port) NULL
#define IP_VS_DBG(level, msg...) do {} while (0)
#define IP_VS_DBG_RL(msg...) do {} while (0)
#define IP_VS_DBG_PKT(level, af, pp, skb, ofs, msg) do {} while (0)
@@ -1244,31 +1255,31 @@ static inline void ip_vs_control_del(struct ip_vs_conn *cp)
{
struct ip_vs_conn *ctl_cp = cp->control;
if (!ctl_cp) {
- IP_VS_ERR_BUF("request control DEL for uncontrolled: "
- "%s:%d to %s:%d\n",
- IP_VS_DBG_ADDR(cp->af, &cp->caddr),
- ntohs(cp->cport),
- IP_VS_DBG_ADDR(cp->af, &cp->vaddr),
- ntohs(cp->vport));
+ pr_err("request control DEL for uncontrolled: "
+ "%pISp to %pISp\n",
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr,
+ ntohs(cp->cport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->vaddr,
+ ntohs(cp->vport)));
return;
}
- IP_VS_DBG_BUF(7, "DELeting control for: "
- "cp.dst=%s:%d ctl_cp.dst=%s:%d\n",
- IP_VS_DBG_ADDR(cp->af, &cp->caddr),
- ntohs(cp->cport),
- IP_VS_DBG_ADDR(cp->af, &ctl_cp->caddr),
- ntohs(ctl_cp->cport));
+ IP_VS_DBG(7, "DELeting control for: "
+ "cp.dst=%pISp ctl_cp.dst=%pISp\n",
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr,
+ ntohs(cp->cport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &ctl_cp->caddr,
+ ntohs(ctl_cp->cport)));
cp->control = NULL;
if (atomic_read(&ctl_cp->n_control) == 0) {
- IP_VS_ERR_BUF("BUG control DEL with n=0 : "
- "%s:%d to %s:%d\n",
- IP_VS_DBG_ADDR(cp->af, &cp->caddr),
- ntohs(cp->cport),
- IP_VS_DBG_ADDR(cp->af, &cp->vaddr),
- ntohs(cp->vport));
+ pr_err("BUG control DEL with n=0 : "
+ "%pISp to %pISp\n",
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr,
+ ntohs(cp->cport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->vaddr,
+ ntohs(cp->vport)));
return;
}
@@ -1279,22 +1290,22 @@ static inline void
ip_vs_control_add(struct ip_vs_conn *cp, struct ip_vs_conn *ctl_cp)
{
if (cp->control) {
- IP_VS_ERR_BUF("request control ADD for already controlled: "
- "%s:%d to %s:%d\n",
- IP_VS_DBG_ADDR(cp->af, &cp->caddr),
- ntohs(cp->cport),
- IP_VS_DBG_ADDR(cp->af, &cp->vaddr),
- ntohs(cp->vport));
+ pr_err("request control ADD for already controlled: "
+ "%pISp to %pISp\n",
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr,
+ ntohs(cp->cport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->vaddr,
+ ntohs(cp->vport)));
ip_vs_control_del(cp);
}
- IP_VS_DBG_BUF(7, "ADDing control for: "
- "cp.dst=%s:%d ctl_cp.dst=%s:%d\n",
- IP_VS_DBG_ADDR(cp->af, &cp->caddr),
- ntohs(cp->cport),
- IP_VS_DBG_ADDR(cp->af, &ctl_cp->caddr),
- ntohs(ctl_cp->cport));
+ IP_VS_DBG(7, "ADDing control for: "
+ "cp.dst=%pISp ctl_cp.dst=%pISp\n",
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr,
+ ntohs(cp->cport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &ctl_cp->caddr,
+ ntohs(ctl_cp->cport)));
cp->control = ctl_cp;
atomic_inc(&ctl_cp->n_control);
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index f662f198b458..0277cd3c5446 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -51,7 +51,6 @@
#include <net/ip_vs.h>
#include <linux/indirect_call_wrapper.h>
-
EXPORT_SYMBOL(register_ip_vs_scheduler);
EXPORT_SYMBOL(unregister_ip_vs_scheduler);
EXPORT_SYMBOL(ip_vs_proto_name);
@@ -294,11 +293,11 @@ ip_vs_sched_persist(struct ip_vs_service *svc,
#endif
snet.ip = src_addr->ip & svc->netmask;
- IP_VS_DBG_BUF(6, "p-schedule: src %s:%u dest %s:%u "
- "mnet %s\n",
- IP_VS_DBG_ADDR(svc->af, src_addr), ntohs(src_port),
- IP_VS_DBG_ADDR(svc->af, dst_addr), ntohs(dst_port),
- IP_VS_DBG_ADDR(svc->af, &snet));
+ IP_VS_DBG(6, "p-schedule: src %pISp dest %pISp "
+ "mnet %pIS\n",
+ IP_VS_DBG_SOCKADDR(svc->af, src_addr, ntohs(src_port)),
+ IP_VS_DBG_SOCKADDR(svc->af, dst_addr, ntohs(dst_port)),
+ IP_VS_DBG_SOCKADDR(svc->af, &snet, 0));
/*
* As far as we know, FTP is a very complicated network protocol, and
@@ -566,12 +565,12 @@ ip_vs_schedule(struct ip_vs_service *svc, struct sk_buff *skb,
}
}
- IP_VS_DBG_BUF(6, "Schedule fwd:%c c:%s:%u v:%s:%u "
- "d:%s:%u conn->flags:%X conn->refcnt:%d\n",
+ IP_VS_DBG(6, "Schedule fwd:%c c:%pISp v:%pISp "
+ "d:%pISp conn->flags:%X conn->refcnt:%d\n",
ip_vs_fwd_tag(cp),
- IP_VS_DBG_ADDR(cp->af, &cp->caddr), ntohs(cp->cport),
- IP_VS_DBG_ADDR(cp->af, &cp->vaddr), ntohs(cp->vport),
- IP_VS_DBG_ADDR(cp->daf, &cp->daddr), ntohs(cp->dport),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr, ntohs(cp->cport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->vaddr, ntohs(cp->vport)),
+ IP_VS_DBG_SOCKADDR(cp->daf, &cp->daddr, ntohs(cp->dport)),
cp->flags, refcount_read(&cp->refcnt));
ip_vs_conn_stats(cp, svc);
@@ -885,8 +884,8 @@ static int handle_response_icmp(int af, struct sk_buff *skb,
/* Ensure the checksum is correct */
if (!skb_csum_unnecessary(skb) && ip_vs_checksum_complete(skb, ihl)) {
/* Failed checksum! */
- IP_VS_DBG_BUF(1, "Forward ICMP: failed checksum from %s!\n",
- IP_VS_DBG_ADDR(af, snet));
+ IP_VS_DBG(1, "Forward ICMP: failed checksum from %pISp!\n",
+ IP_VS_DBG_SOCKADDR(af, snet, 0));
goto out;
}
@@ -1219,13 +1218,13 @@ struct ip_vs_conn *ip_vs_new_conn_out(struct ip_vs_service *svc,
ip_vs_conn_stats(cp, svc);
/* return connection (will be used to handle outgoing packet) */
- IP_VS_DBG_BUF(6, "New connection RS-initiated:%c c:%s:%u v:%s:%u "
- "d:%s:%u conn->flags:%X conn->refcnt:%d\n",
- ip_vs_fwd_tag(cp),
- IP_VS_DBG_ADDR(cp->af, &cp->caddr), ntohs(cp->cport),
- IP_VS_DBG_ADDR(cp->af, &cp->vaddr), ntohs(cp->vport),
- IP_VS_DBG_ADDR(cp->af, &cp->daddr), ntohs(cp->dport),
- cp->flags, refcount_read(&cp->refcnt));
+ IP_VS_DBG(6, "New connection RS-initiated:%c c:%pISp v:%pISp "
+ "d:%pISp conn->flags:%X conn->refcnt:%d\n",
+ ip_vs_fwd_tag(cp),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr, ntohs(cp->cport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->vaddr, ntohs(cp->vport)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->daddr, ntohs(cp->dport)),
+ cp->flags, refcount_read(&cp->refcnt));
LeaveFunction(12);
return cp;
}
@@ -1931,7 +1930,6 @@ static int ip_vs_in_icmp_v6(struct netns_ipvs *ipvs, struct sk_buff *skb,
}
#endif
-
/*
* Check if it's for virtual services, look it up,
* and send it on its way...
@@ -1960,10 +1958,10 @@ ip_vs_in(struct netns_ipvs *ipvs, unsigned int hooknum, struct sk_buff *skb, int
hooknum != NF_INET_LOCAL_OUT) ||
!skb_dst(skb))) {
ip_vs_fill_iph_skb(af, skb, false, &iph);
- IP_VS_DBG_BUF(12, "packet type=%d proto=%d daddr=%s"
+ IP_VS_DBG(12, "packet type=%d proto=%d daddr=%pIS"
" ignored in hook %u\n",
skb->pkt_type, iph.protocol,
- IP_VS_DBG_ADDR(af, &iph.daddr), hooknum);
+ IP_VS_DBG_SOCKADDR(af, &iph.daddr, 0), hooknum);
return NF_ACCEPT;
}
/* ipvs enabled in this netns ? */
diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
index cf925906f59b..d57dcc2b4396 100644
--- a/net/netfilter/ipvs/ip_vs_ftp.c
+++ b/net/netfilter/ipvs/ip_vs_ftp.c
@@ -306,9 +306,9 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp,
&start, &end) != 1)
return 1;
- IP_VS_DBG_BUF(7, "EPSV response (%s:%u) -> %s:%u detected\n",
- IP_VS_DBG_ADDR(cp->af, &from), ntohs(port),
- IP_VS_DBG_ADDR(cp->af, &cp->caddr), 0);
+ IP_VS_DBG(7, "EPSV response (%pISp) -> %pISp detected\n",
+ IP_VS_DBG_SOCKADDR(cp->af, &from, ntohs(port)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->caddr, 0));
} else {
return 1;
}
@@ -510,15 +510,15 @@ static int ip_vs_ftp_in(struct ip_vs_app *app, struct ip_vs_conn *cp,
&to, &port, cp->af,
&start, &end) == 1) {
- IP_VS_DBG_BUF(7, "EPRT %s:%u detected\n",
- IP_VS_DBG_ADDR(cp->af, &to), ntohs(port));
+ IP_VS_DBG(7, "EPRT %pISp detected\n",
+ IP_VS_DBG_SOCKADDR(cp->af, &to, ntohs(port)));
/* Now update or create a connection entry for it */
- IP_VS_DBG_BUF(7, "protocol %s %s:%u %s:%u\n",
- ip_vs_proto_name(ipvsh->protocol),
- IP_VS_DBG_ADDR(cp->af, &to), ntohs(port),
- IP_VS_DBG_ADDR(cp->af, &cp->vaddr),
- ntohs(cp->vport)-1);
+ IP_VS_DBG(7, "protocol %s %pISp %pISp\n",
+ ip_vs_proto_name(ipvsh->protocol),
+ IP_VS_DBG_SOCKADDR(cp->af, &to, ntohs(port)),
+ IP_VS_DBG_SOCKADDR(cp->af, &cp->vaddr,
+ ntohs(cp->vport)-1));
} else {
return 1;
}
--
2.20.0
On Fri, Jun 28, 2019 at 02:37:46PM +0200, Arnd Bergmann wrote:
> The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF
> leads to much larger kernel stack usage, as seen from the warnings
> about functions that now exceed the 2048 byte limit:
>
> drivers/media/i2c/tvp5150.c:253:1: error: the frame size of 3936 bytes is larger than 2048 bytes
> drivers/media/tuners/r820t.c:1327:1: error: the frame size of 2816 bytes is larger than 2048 bytes
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:16552:1: error: the frame size of 3144 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> fs/ocfs2/aops.c:1892:1: error: the frame size of 2088 bytes is larger than 2048 bytes
> fs/ocfs2/dlm/dlmrecovery.c:737:1: error: the frame size of 2088 bytes is larger than 2048 bytes
> fs/ocfs2/namei.c:1677:1: error: the frame size of 2584 bytes is larger than 2048 bytes
> fs/ocfs2/super.c:1186:1: error: the frame size of 2640 bytes is larger than 2048 bytes
> fs/ocfs2/xattr.c:3678:1: error: the frame size of 2176 bytes is larger than 2048 bytes
> net/bluetooth/l2cap_core.c:7056:1: error: the frame size of 2144 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> net/bluetooth/l2cap_core.c: In function 'l2cap_recv_frame':
> net/bridge/br_netlink.c:1505:1: error: the frame size of 2448 bytes is larger than 2048 bytes
> net/ieee802154/nl802154.c:548:1: error: the frame size of 2232 bytes is larger than 2048 bytes
> net/wireless/nl80211.c:1726:1: error: the frame size of 2224 bytes is larger than 2048 bytes
> net/wireless/nl80211.c:2357:1: error: the frame size of 4584 bytes is larger than 2048 bytes
> net/wireless/nl80211.c:5108:1: error: the frame size of 2760 bytes is larger than 2048 bytes
> net/wireless/nl80211.c:6472:1: error: the frame size of 2112 bytes is larger than 2048 bytes
>
> The structleak plugin was previously disabled for CONFIG_COMPILE_TEST,
> but meant we missed some bugs, so this time we should address them.
>
> The frame size warnings are distracting, and risking a kernel stack
> overflow is generally not beneficial to performance, so it may be best
> to disallow that particular combination. This can be done by turning
> off either one. I picked the dependency in GCC_PLUGIN_STRUCTLEAK_BYREF
> and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, as this option is designed to
> make uninitialized stack usage less harmful when enabled on its own,
> but it also prevents KASAN from detecting those cases in which it was
> in fact needed.
>
> KASAN_STACK is currently implied by KASAN on gcc, but could be made a
> user selectable option if we want to allow combining (non-stack) KASAN
> with GCC_PLUGIN_STRUCTLEAK_BYREF.
>
> Note that it would be possible to specifically address the files that
> print the warning, but presumably the overall stack usage is still
> significantly higher than in other configurations, so this would not
> address the full problem.
>
> I could not test this with CONFIG_INIT_STACK_ALL, which may or may not
> suffer from a similar problem.
>
> Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
> Signed-off-by: Arnd Bergmann <[email protected]>
Acked-by: Kees Cook <[email protected]>
-Kees
> ---
> [v2] do it for both GCC_PLUGIN_STRUCTLEAK_BYREF and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
> ---
> security/Kconfig.hardening | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index a1ffe2eb4d5f..af4c979b38ee 100644
> --- a/security/Kconfig.hardening
> +++ b/security/Kconfig.hardening
> @@ -61,6 +61,7 @@ choice
> config GCC_PLUGIN_STRUCTLEAK_BYREF
> bool "zero-init structs passed by reference (strong)"
> depends on GCC_PLUGINS
> + depends on !(KASAN && KASAN_STACK=1)
> select GCC_PLUGIN_STRUCTLEAK
> help
> Zero-initialize any structures on the stack that may
> @@ -70,9 +71,15 @@ choice
> exposures, like CVE-2017-1000410:
> https://git.kernel.org/linus/06e7e776ca4d3654
>
> + As a side-effect, this keeps a lot of variables on the
> + stack that can otherwise be optimized out, so combining
> + this with CONFIG_KASAN_STACK can lead to a stack overflow
> + and is disallowed.
> +
> config GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> bool "zero-init anything passed by reference (very strong)"
> depends on GCC_PLUGINS
> + depends on !(KASAN && KASAN_STACK=1)
> select GCC_PLUGIN_STRUCTLEAK
> help
> Zero-initialize any stack variables that may be passed
> --
> 2.20.0
>
--
Kees Cook
On 6/28/2019 5:37 AM, Arnd Bergmann wrote:
> The lpfc_debug_dump_all_queues() function repeatedly calls into
> lpfc_debug_dump_qe(), which has a temporary 128 byte buffer.
> This was fine before the introduction of CONFIG_GCC_PLUGIN_STRUCTLEAK_VERBOSE
> because each instance could occupy the same stack slot. However,
> now they each get their own copy, which leads to a huge increase
> in stack usage as seen from the compiler warning:
>
> drivers/scsi/lpfc/lpfc_debugfs.c: In function 'lpfc_debug_dump_all_queues':
> drivers/scsi/lpfc/lpfc_debugfs.c:6474:1: error: the frame size of 1712 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
>
> Avoid this by not marking lpfc_debug_dump_qe() as inline so the
> compiler can choose to emit a static version of this function
> when it's needed or otherwise silently drop it. As an added benefit,
> not inlining multiple copies of this function means we save several
> kilobytes of .text section, reducing the file size from 47kb to 43.
>
> It is somewhat unusual to have a function that is static but not
> inline in a header file, but this does not cause problems here
> because it is only used by other inline functions. It would
> however seem reasonable to move all the lpfc_debug_dump_* functions
> into lpfc_debugfs.c and not mark them inline as a later cleanup.
I agree with this cleanup.
>
> Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/scsi/lpfc/lpfc_debugfs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
Reviewed-by: James Smart <[email protected]>
-- james
On Fri, Jun 28, 2019 at 8:41 AM Arnd Bergmann <[email protected]> wrote:
>
> An earlier patch I sent reduced the stack usage enough to get
> below the warning limit, and I could show this was safe, but with
> GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, it gets worse again because large stack
> variables in the same function no longer overlap:
>
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'translate_scan.isra.2':
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:322:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Split out the largest two blocks in the affected function into two
> separate functions and mark those noinline_for_stack.
>
> Fixes: 8c5af16f7953 ("staging: rtl8712: reduce stack usage")
> Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Willem de Bruijn <[email protected]>
On Fri, Jun 28, 2019 at 8:40 AM Arnd Bergmann <[email protected]> wrote:
>
> With the new CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL option, the stack
> usage in the ipvs debug output grows because each instance of
> IP_VS_DBG_BUF() now has its own buffer of 160 bytes that add up
> rather than reusing the stack slots:
>
> net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_sched_persist':
> net/netfilter/ipvs/ip_vs_core.c:427:1: error: the frame size of 1052 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_new_conn_out':
> net/netfilter/ipvs/ip_vs_core.c:1231:1: error: the frame size of 1048 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_out':
> net/netfilter/ipvs/ip_vs_ftp.c:397:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_in':
> net/netfilter/ipvs/ip_vs_ftp.c:555:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Since printk() already has a way to print IPv4/IPv6 addresses using
> the %pIS format string, use that instead,
since these are sockaddr_in and sockaddr_in6, should that have the 'n'
specifier to denote network byteorder?
> combined with a macro that
> creates a local sockaddr structure on the stack. These will still
> add up, but the stack frames are now under 200 bytes.
would it make sense to just define a helper function that takes const
char * level and msg strings and up to three struct nf_inet_addr* and
do the conversion in there? No need for macros and no state on the
stack outside error paths at all.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> I'm not sure this actually does what I think it does. Someone
> needs to verify that we correctly print the addresses here.
> I've also only added three files that caused the warning messages
> to be reported. There are still a lot of other instances of
> IP_VS_DBG_BUF() that could be converted the same way after the
> basic idea is confirmed.
> ---
> include/net/ip_vs.h | 71 +++++++++++++++++++--------------
> net/netfilter/ipvs/ip_vs_core.c | 44 ++++++++++----------
> net/netfilter/ipvs/ip_vs_ftp.c | 20 +++++-----
> 3 files changed, 72 insertions(+), 63 deletions(-)
>
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 3759167f91f5..3dfbeef67be6 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -227,6 +227,16 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
> sizeof(ip_vs_dbg_buf), addr, \
> &ip_vs_dbg_idx)
>
> +#define IP_VS_DBG_SOCKADDR4(fam, addr, port) \
> + (struct sockaddr*)&(struct sockaddr_in) \
> + { .sin_family = (fam), .sin_addr = (addr)->in, .sin_port = (port) }
might as well set .sin_family = AF_INET here and AF_INET6 below?
> +#define IP_VS_DBG_SOCKADDR6(fam, addr, port) \
> + (struct sockaddr*)&(struct sockaddr_in6) \
> + { .sin6_family = (fam), .sin6_addr = (addr)->in6, .sin6_port = (port) }
Hello,
On Fri, 28 Jun 2019, Arnd Bergmann wrote:
> With the new CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL option, the stack
> usage in the ipvs debug output grows because each instance of
> IP_VS_DBG_BUF() now has its own buffer of 160 bytes that add up
> rather than reusing the stack slots:
>
> net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_sched_persist':
> net/netfilter/ipvs/ip_vs_core.c:427:1: error: the frame size of 1052 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_new_conn_out':
> net/netfilter/ipvs/ip_vs_core.c:1231:1: error: the frame size of 1048 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_out':
> net/netfilter/ipvs/ip_vs_ftp.c:397:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_in':
> net/netfilter/ipvs/ip_vs_ftp.c:555:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Since printk() already has a way to print IPv4/IPv6 addresses using
> the %pIS format string, use that instead, combined with a macro that
> creates a local sockaddr structure on the stack. These will still
> add up, but the stack frames are now under 200 bytes.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> I'm not sure this actually does what I think it does. Someone
> needs to verify that we correctly print the addresses here.
> I've also only added three files that caused the warning messages
> to be reported. There are still a lot of other instances of
> IP_VS_DBG_BUF() that could be converted the same way after the
> basic idea is confirmed.
> ---
> include/net/ip_vs.h | 71 +++++++++++++++++++--------------
> net/netfilter/ipvs/ip_vs_core.c | 44 ++++++++++----------
> net/netfilter/ipvs/ip_vs_ftp.c | 20 +++++-----
> 3 files changed, 72 insertions(+), 63 deletions(-)
>
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 3759167f91f5..3dfbeef67be6 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -227,6 +227,16 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
> sizeof(ip_vs_dbg_buf), addr, \
> &ip_vs_dbg_idx)
>
> +#define IP_VS_DBG_SOCKADDR4(fam, addr, port) \
> + (struct sockaddr*)&(struct sockaddr_in) \
> + { .sin_family = (fam), .sin_addr = (addr)->in, .sin_port = (port) }
> +#define IP_VS_DBG_SOCKADDR6(fam, addr, port) \
> + (struct sockaddr*)&(struct sockaddr_in6) \
> + { .sin6_family = (fam), .sin6_addr = (addr)->in6, .sin6_port = (port) }
> +#define IP_VS_DBG_SOCKADDR(fam, addr, port) (fam == AF_INET ? \
> + IP_VS_DBG_SOCKADDR4(fam, addr, port) : \
> + IP_VS_DBG_SOCKADDR6(fam, addr, port))
> +
> #define IP_VS_DBG(level, msg, ...) \
> do { \
> if (level <= ip_vs_get_debug_level()) \
> @@ -251,6 +261,7 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
> #else /* NO DEBUGGING at ALL */
> #define IP_VS_DBG_BUF(level, msg...) do {} while (0)
> #define IP_VS_ERR_BUF(msg...) do {} while (0)
> +#define IP_VS_DBG_SOCKADDR(fam, addr, port) NULL
> #define IP_VS_DBG(level, msg...) do {} while (0)
> #define IP_VS_DBG_RL(msg...) do {} while (0)
> #define IP_VS_DBG_PKT(level, af, pp, skb, ofs, msg) do {} while (0)
> @@ -1244,31 +1255,31 @@ static inline void ip_vs_control_del(struct ip_vs_conn *cp)
> {
> struct ip_vs_conn *ctl_cp = cp->control;
> if (!ctl_cp) {
> - IP_VS_ERR_BUF("request control DEL for uncontrolled: "
> - "%s:%d to %s:%d\n",
> - IP_VS_DBG_ADDR(cp->af, &cp->caddr),
> - ntohs(cp->cport),
> - IP_VS_DBG_ADDR(cp->af, &cp->vaddr),
> - ntohs(cp->vport));
> + pr_err("request control DEL for uncontrolled: "
> + "%pISp to %pISp\n",
ip_vs_dbg_addr() used compact form (%pI6c), so it would be
better to use %pISc and %pISpc everywhere in IPVS...
Also, note that before now port was printed with %d and
ntohs() was used, now port should be in network order, so:
- ntohs() should be removed
- htons() should be added, if missing. At first look, this case
is not present in IPVS, we have only ntohs() usage
Regards
--
Julian Anastasov <[email protected]>
Arnd,
> The lpfc_debug_dump_all_queues() function repeatedly calls into
> lpfc_debug_dump_qe(), which has a temporary 128 byte buffer. This was
> fine before the introduction of CONFIG_GCC_PLUGIN_STRUCTLEAK_VERBOSE
> because each instance could occupy the same stack slot. However, now
> they each get their own copy, which leads to a huge increase in stack
> usage as seen from the compiler warning:
Applied to 5.3/scsi-fixes. Thank you!
--
Martin K. Petersen Oracle Linux Engineering
On Sun, Jun 30, 2019 at 10:37 PM Julian Anastasov <[email protected]> wrote:
> On Fri, 28 Jun 2019, Arnd Bergmann wrote:
> > struct ip_vs_conn *ctl_cp = cp->control;
> > if (!ctl_cp) {
> > - IP_VS_ERR_BUF("request control DEL for uncontrolled: "
> > - "%s:%d to %s:%d\n",
> > - IP_VS_DBG_ADDR(cp->af, &cp->caddr),
> > - ntohs(cp->cport),
> > - IP_VS_DBG_ADDR(cp->af, &cp->vaddr),
> > - ntohs(cp->vport));
> > + pr_err("request control DEL for uncontrolled: "
> > + "%pISp to %pISp\n",
(replying a bit late)
> ip_vs_dbg_addr() used compact form (%pI6c), so it would be
> better to use %pISc and %pISpc everywhere in IPVS...
done
> Also, note that before now port was printed with %d and
> ntohs() was used, now port should be in network order, so:
>
> - ntohs() should be removed
done
> - htons() should be added, if missing. At first look, this case
> is not present in IPVS, we have only ntohs() usage
I found one case in ip_vs_ftp_in() that needs it in order to subtract one:
IP_VS_DBG(7, "protocol %s %pISpc %pISpc\n",
ip_vs_proto_name(ipvsh->protocol),
- IP_VS_DBG_SOCKADDR(cp->af, &to, ntohs(port)),
+ IP_VS_DBG_SOCKADDR(cp->af, &to, port),
IP_VS_DBG_SOCKADDR(cp->af, &cp->vaddr,
- ntohs(cp->vport)-1));
+ htons(ntohs(cp->vport)-1)));
Thanks for the review, I'll send a new patch after some more
build testing on the new version.
Arnd
On Fri, Jun 28, 2019 at 9:59 PM Willem de Bruijn
<[email protected]> wrote:
> On Fri, Jun 28, 2019 at 8:40 AM Arnd Bergmann <[email protected]> wrote:
> >
> > With the new CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL option, the stack
> > usage in the ipvs debug output grows because each instance of
> > IP_VS_DBG_BUF() now has its own buffer of 160 bytes that add up
> > rather than reusing the stack slots:
> >
> > net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_sched_persist':
> > net/netfilter/ipvs/ip_vs_core.c:427:1: error: the frame size of 1052 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> > net/netfilter/ipvs/ip_vs_core.c: In function 'ip_vs_new_conn_out':
> > net/netfilter/ipvs/ip_vs_core.c:1231:1: error: the frame size of 1048 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> > net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_out':
> > net/netfilter/ipvs/ip_vs_ftp.c:397:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> > net/netfilter/ipvs/ip_vs_ftp.c: In function 'ip_vs_ftp_in':
> > net/netfilter/ipvs/ip_vs_ftp.c:555:1: error: the frame size of 1200 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> >
> > Since printk() already has a way to print IPv4/IPv6 addresses using
> > the %pIS format string, use that instead,
>
> since these are sockaddr_in and sockaddr_in6, should that have the 'n'
> specifier to denote network byteorder?
I double-checked the implementation, and don't see that make any difference,
as 'n' is the default.
> > include/net/ip_vs.h | 71 +++++++++++++++++++--------------
> > net/netfilter/ipvs/ip_vs_core.c | 44 ++++++++++----------
> > net/netfilter/ipvs/ip_vs_ftp.c | 20 +++++-----
> > 3 files changed, 72 insertions(+), 63 deletions(-)
> >
> > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > index 3759167f91f5..3dfbeef67be6 100644
> > --- a/include/net/ip_vs.h
> > +++ b/include/net/ip_vs.h
> > @@ -227,6 +227,16 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
> > sizeof(ip_vs_dbg_buf), addr, \
> > &ip_vs_dbg_idx)
> >
> > +#define IP_VS_DBG_SOCKADDR4(fam, addr, port) \
> > + (struct sockaddr*)&(struct sockaddr_in) \
> > + { .sin_family = (fam), .sin_addr = (addr)->in, .sin_port = (port) }
>
> might as well set .sin_family = AF_INET here and AF_INET6 below?
That would work just same, but I don't see any advantage. As the family
and port members are the same between sockaddr_in and sockaddr_in6,
the compiler can decide to set these regardless to the argument values
regardless of the condition.
Arnd