| Message ID | d277733a2eb3b5ffa2cbdbe4bd2a261d8fb60a0f.1531811764.git.sbrivio@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: Move skb decrypted field, avoid explicity copy | expand |
Hi Stefano,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Stefano-Brivio/net-Move-skb-decrypted-field-avoid-explicity-copy/20180717-152125
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=ia64
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:10,
from include/linux/list.h:9,
from include/linux/module.h:9,
from net//core/skbuff.c:41:
net//core/skbuff.c: In function '__copy_skb_header':
>> net//core/skbuff.c:787:31: error: attempt to take address of bit-field structure member 'decrypted'
BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
^~~~~~~
include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^~~~~~~~~
include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
net//core/skbuff.c:787:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
^~~~~~~~~~~~
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^~~~~~~~~~~~~~~~~~~
net//core/skbuff.c:787:15: note: in expansion of macro 'offsetof'
BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
^~~~~~~~
net//core/skbuff.c:837:2: note: in expansion of macro 'CHECK_SKB_FIELD'
CHECK_SKB_FIELD(decrypted);
^~~~~~~~~~~~~~~
net//core/skbuff.c:789:31: error: attempt to take address of bit-field structure member 'decrypted'
BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
^~~~~~~
include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^~~~~~~~~
include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
net//core/skbuff.c:789:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
^~~~~~~~~~~~
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^~~~~~~~~~~~~~~~~~~
net//core/skbuff.c:789:15: note: in expansion of macro 'offsetof'
BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
^~~~~~~~
net//core/skbuff.c:837:2: note: in expansion of macro 'CHECK_SKB_FIELD'
CHECK_SKB_FIELD(decrypted);
^~~~~~~~~~~~~~~
vim +/decrypted +787 net//core/skbuff.c
795bb1c0 Jesper Dangaard Brouer 2016-02-08 784
b1937227 Eric Dumazet 2014-09-28 785 /* Make sure a field is enclosed inside headers_start/headers_end section */
b1937227 Eric Dumazet 2014-09-28 786 #define CHECK_SKB_FIELD(field) \
b1937227 Eric Dumazet 2014-09-28 @787 BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
b1937227 Eric Dumazet 2014-09-28 788 offsetof(struct sk_buff, headers_start)); \
b1937227 Eric Dumazet 2014-09-28 789 BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
b1937227 Eric Dumazet 2014-09-28 790 offsetof(struct sk_buff, headers_end)); \
b1937227 Eric Dumazet 2014-09-28 791
:::::: The code at line 787 was first introduced by commit
:::::: b1937227316417aa7568d01e6fa1f272e98fb890 net: reorganize sk_buff for faster __copy_skb_header()
:::::: TO: Eric Dumazet <edumazet@google.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Stefano,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Stefano-Brivio/net-Move-skb-decrypted-field-avoid-explicity-copy/20180717-152125
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/module.h:9,
from net//core/skbuff.c:41:
net//core/skbuff.c: In function '__copy_skb_header':
net//core/skbuff.c:787:31: error: attempt to take address of bit-field structure member 'decrypted'
BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
^
include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^~~~~~~~~
include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
>> net//core/skbuff.c:787:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
^~~~~~~~~~~~
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^~~~~~~~~~~~~~~~~~~
>> net//core/skbuff.c:787:15: note: in expansion of macro 'offsetof'
BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
^~~~~~~~
>> net//core/skbuff.c:837:2: note: in expansion of macro 'CHECK_SKB_FIELD'
CHECK_SKB_FIELD(decrypted);
^~~~~~~~~~~~~~~
net//core/skbuff.c:789:31: error: attempt to take address of bit-field structure member 'decrypted'
BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
^
include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^~~~~~~~~
include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
net//core/skbuff.c:789:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
^~~~~~~~~~~~
include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^~~~~~~~~~~~~~~~~~~
net//core/skbuff.c:789:15: note: in expansion of macro 'offsetof'
BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
^~~~~~~~
>> net//core/skbuff.c:837:2: note: in expansion of macro 'CHECK_SKB_FIELD'
CHECK_SKB_FIELD(decrypted);
^~~~~~~~~~~~~~~
vim +/CHECK_SKB_FIELD +837 net//core/skbuff.c
784
785 /* Make sure a field is enclosed inside headers_start/headers_end section */
786 #define CHECK_SKB_FIELD(field) \
> 787 BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
788 offsetof(struct sk_buff, headers_start)); \
789 BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
790 offsetof(struct sk_buff, headers_end)); \
791
792 static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
793 {
794 new->tstamp = old->tstamp;
795 /* We do not copy old->sk */
796 new->dev = old->dev;
797 memcpy(new->cb, old->cb, sizeof(old->cb));
798 skb_dst_copy(new, old);
799 #ifdef CONFIG_XFRM
800 new->sp = secpath_get(old->sp);
801 #endif
802 __nf_copy(new, old, false);
803
804 /* Note : this field could be in headers_start/headers_end section
805 * It is not yet because we do not want to have a 16 bit hole
806 */
807 new->queue_mapping = old->queue_mapping;
808
809 memcpy(&new->headers_start, &old->headers_start,
810 offsetof(struct sk_buff, headers_end) -
811 offsetof(struct sk_buff, headers_start));
812 CHECK_SKB_FIELD(protocol);
813 CHECK_SKB_FIELD(csum);
814 CHECK_SKB_FIELD(hash);
815 CHECK_SKB_FIELD(priority);
816 CHECK_SKB_FIELD(skb_iif);
817 CHECK_SKB_FIELD(vlan_proto);
818 CHECK_SKB_FIELD(vlan_tci);
819 CHECK_SKB_FIELD(transport_header);
820 CHECK_SKB_FIELD(network_header);
821 CHECK_SKB_FIELD(mac_header);
822 CHECK_SKB_FIELD(inner_protocol);
823 CHECK_SKB_FIELD(inner_transport_header);
824 CHECK_SKB_FIELD(inner_network_header);
825 CHECK_SKB_FIELD(inner_mac_header);
826 CHECK_SKB_FIELD(mark);
827 #ifdef CONFIG_NETWORK_SECMARK
828 CHECK_SKB_FIELD(secmark);
829 #endif
830 #ifdef CONFIG_NET_RX_BUSY_POLL
831 CHECK_SKB_FIELD(napi_id);
832 #endif
833 #ifdef CONFIG_XPS
834 CHECK_SKB_FIELD(sender_cpu);
835 #endif
836 #ifdef CONFIG_TLS_DEVICE
> 837 CHECK_SKB_FIELD(decrypted);
838 #endif
839 #ifdef CONFIG_NET_SCHED
840 CHECK_SKB_FIELD(tc_index);
841 #endif
842
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 3ceb8dcc54da..14bc9ebe30f2 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -630,7 +630,6 @@ typedef unsigned char *sk_buff_data_t; * @hash: the packet hash * @queue_mapping: Queue mapping for multiqueue devices * @xmit_more: More SKBs are pending for this queue - * @decrypted: Decrypted SKB * @ndisc_nodetype: router type (from link layer) * @ooo_okay: allow the mapping of a socket to a queue to be changed * @l4_hash: indicate hash is a canonical 4-tuple hash over transport @@ -641,6 +640,7 @@ typedef unsigned char *sk_buff_data_t; * @no_fcs: Request NIC to treat last 4 bytes as Ethernet FCS * @csum_not_inet: use CRC32c to resolve CHECKSUM_PARTIAL * @dst_pending_confirm: need to confirm neighbour + * @decrypted: Decrypted SKB * @napi_id: id of the NAPI struct this skb came from * @secmark: security marking * @mark: Generic packet mark @@ -737,11 +737,7 @@ struct sk_buff { peeked:1, head_frag:1, xmit_more:1, -#ifdef CONFIG_TLS_DEVICE - decrypted:1; -#else __unused:1; -#endif /* fields enclosed in headers_start/headers_end are copied * using a single memcpy() in __copy_skb_header() @@ -797,6 +793,9 @@ struct sk_buff { __u8 tc_redirected:1; __u8 tc_from_ingress:1; #endif +#ifdef CONFIG_TLS_DEVICE + __u8 decrypted:1; +#endif #ifdef CONFIG_NET_SCHED __u16 tc_index; /* traffic control index */ diff --git a/net/core/skbuff.c b/net/core/skbuff.c index cfd6c6f35f9c..b8a563b2d2df 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -805,9 +805,6 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) * It is not yet because we do not want to have a 16 bit hole */ new->queue_mapping = old->queue_mapping; -#ifdef CONFIG_TLS_DEVICE - new->decrypted = old->decrypted; -#endif memcpy(&new->headers_start, &old->headers_start, offsetof(struct sk_buff, headers_end) - @@ -836,6 +833,9 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) #ifdef CONFIG_XPS CHECK_SKB_FIELD(sender_cpu); #endif +#ifdef CONFIG_TLS_DEVICE + CHECK_SKB_FIELD(decrypted); +#endif #ifdef CONFIG_NET_SCHED CHECK_SKB_FIELD(tc_index); #endif @@ -868,9 +868,6 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb) C(head_frag); C(data); C(truesize); -#ifdef CONFIG_TLS_DEVICE - C(decrypted); -#endif refcount_set(&n->users, 1); atomic_inc(&(skb_shinfo(skb)->dataref));
Commit 784abe24c903 ("net: Add decrypted field to skb") introduced a 'decrypted' field that is explicitly copied on skb copy and clone. Move it between headers_start[0] and headers_end[0], so that we don't need to copy it explicitly as it's copied by the memcpy() in __copy_skb_header(). While at it, drop the assignment in __skb_clone(), it was already redundant. This doesn't change the size of sk_buff or cacheline boundaries. The 15-bits hole before tc_index becomes a 14-bits hole, and will be again a 15-bits hole when this change is merged with commit 8b7008620b84 ("net: Don't copy pfmemalloc flag in __copy_skb_header()"). Fixes: 784abe24c903 ("net: Add decrypted field to skb") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- include/linux/skbuff.h | 9 ++++----- net/core/skbuff.c | 9 +++------ 2 files changed, 7 insertions(+), 11 deletions(-)