* [PATCH 0/3] skbuff: pskb_expand_head changes @ 2013-06-12 9:05 Dave Wiltshire 2013-06-12 9:05 ` [PATCH 1/3] skbuff: Update truesize in pskb_expand_head Dave Wiltshire 0 siblings, 1 reply; 3+ messages in thread From: Dave Wiltshire @ 2013-06-12 9:05 UTC (permalink / raw) To: davem Cc: netdev, linux-kernel, nsujir, mchan, rmody, jcliburn, chris.snook, jeffrey.t.kirsher, bruce.w.allan, alexander.h.duyck, cooldavid, linux-driver, linux-wimax, wimax, linux-wireless, viro, eparis, edumazet, dev, jhs, Dave Wiltshire This patchset applies against David Miller's net-next tree. This is my first patch, so please let me know if I've done something wrong or could do something better (e.g. splitting the patch in a different way). I think there is a problem with memory accounting in pskb_expand_head. Some call sites update truesize while others don't. This means that the skbuff can change in size without truesize being updated. This seems incorrect to me. So I changed it. Perhaps I don't understand something, but I thought it best to generate the change and then ask. So is this correct? The other patches are from looking at all the call sites to pskb_expand_head and since I was doing that I cleaned some of them up. Dave Wiltshire (3): skbuff: Update truesize in pskb_expand_head skbuff: skb_cow_head not used in some drivers. skbuff: Added new helper function skb_cow_clone_head. drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 8 ++------ drivers/net/ethernet/atheros/atl1e/atl1e_main.c | 8 ++------ drivers/net/ethernet/atheros/atlx/atl1.c | 8 ++------ drivers/net/ethernet/broadcom/tg3.c | 3 +-- drivers/net/ethernet/brocade/bna/bnad.c | 11 +++-------- drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------ drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++------ drivers/net/ethernet/intel/igb/igb_main.c | 7 ++----- drivers/net/ethernet/intel/igbvf/netdev.c | 11 +++-------- drivers/net/ethernet/intel/ixgb/ixgb_main.c | 8 ++------ drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 +++------- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 7 ++----- drivers/net/ethernet/jme.c | 3 +-- drivers/net/ethernet/qlogic/qlge/qlge_main.c | 8 ++------ drivers/net/wimax/i2400m/netdev.c | 3 +-- drivers/net/wireless/mwl8k.c | 1 - include/linux/skbuff.h | 14 +++++++++++++ kernel/audit.c | 2 -- net/core/dev.c | 8 ++------ net/core/skbuff.c | 1 + net/netlink/af_netlink.c | 3 +-- net/openvswitch/actions.c | 22 +++++++-------------- net/sched/act_csum.c | 8 ++------ net/sched/act_nat.c | 18 +++++------------ net/wireless/util.c | 2 -- 25 files changed, 62 insertions(+), 128 deletions(-) -- 1.7.10.4 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/3] skbuff: Update truesize in pskb_expand_head 2013-06-12 9:05 [PATCH 0/3] skbuff: pskb_expand_head changes Dave Wiltshire @ 2013-06-12 9:05 ` Dave Wiltshire 2013-06-12 9:16 ` Eric Dumazet 0 siblings, 1 reply; 3+ messages in thread From: Dave Wiltshire @ 2013-06-12 9:05 UTC (permalink / raw) To: davem Cc: netdev, linux-kernel, nsujir, mchan, rmody, jcliburn, chris.snook, jeffrey.t.kirsher, bruce.w.allan, alexander.h.duyck, cooldavid, linux-driver, linux-wimax, wimax, linux-wireless, viro, eparis, edumazet, dev, jhs, Dave Wiltshire Some call sites to pskb_expand_head subsequently update the skb truesize and others don't (even with non-zero arguments). This is likely a memory audit leak. Fixed this up by moving the memory accounting to the skbuff.c file and removing it from the calling sites. Signed-off-by: Dave Wiltshire <david.wiltshire@gmx.com> --- drivers/net/wireless/mwl8k.c | 1 - kernel/audit.c | 2 -- net/core/skbuff.c | 1 + net/netlink/af_netlink.c | 3 +-- net/wireless/util.c | 2 -- 5 files changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c index 6820fce..802c8d7 100644 --- a/drivers/net/wireless/mwl8k.c +++ b/drivers/net/wireless/mwl8k.c @@ -845,7 +845,6 @@ mwl8k_add_dma_header(struct mwl8k_priv *priv, struct sk_buff *skb, "Failed to reallocate TX buffer\n"); return; } - skb->truesize += REDUCED_TX_HEADROOM; } reqd_hdrlen = sizeof(*tr) + head_pad; diff --git a/kernel/audit.c b/kernel/audit.c index 21c7fa6..e05b57b 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1157,7 +1157,6 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, static inline int audit_expand(struct audit_buffer *ab, int extra) { struct sk_buff *skb = ab->skb; - int oldtail = skb_tailroom(skb); int ret = pskb_expand_head(skb, 0, extra, ab->gfp_mask); int newtail = skb_tailroom(skb); @@ -1166,7 +1165,6 @@ static inline int audit_expand(struct audit_buffer *ab, int extra) return 0; } - skb->truesize += newtail - oldtail; return newtail; } diff --git a/net/core/skbuff.c b/net/core/skbuff.c index edf3757..125bb7e 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -1061,6 +1061,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, if (!data) goto nodata; size = SKB_WITH_OVERHEAD(ksize(data)); + skb->truesize += size - skb_end_offset(skb); /* Copy only real data... and, alas, header. This should be * optimized for the cases when header is void. diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 9b6b115..77fd986 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1565,8 +1565,7 @@ static struct sk_buff *netlink_trim(struct sk_buff *skb, gfp_t allocation) skb = nskb; } - if (!pskb_expand_head(skb, 0, -delta, allocation)) - skb->truesize -= delta; + pskb_expand_head(skb, 0, -delta, allocation); return skb; } diff --git a/net/wireless/util.c b/net/wireless/util.c index f5ad4d9..5710aa2 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -533,8 +533,6 @@ int ieee80211_data_from_8023(struct sk_buff *skb, const u8 *addr, if (pskb_expand_head(skb, head_need, 0, GFP_ATOMIC)) return -ENOMEM; - - skb->truesize += head_need; } if (encaps_data) { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] skbuff: Update truesize in pskb_expand_head 2013-06-12 9:05 ` [PATCH 1/3] skbuff: Update truesize in pskb_expand_head Dave Wiltshire @ 2013-06-12 9:16 ` Eric Dumazet 0 siblings, 0 replies; 3+ messages in thread From: Eric Dumazet @ 2013-06-12 9:16 UTC (permalink / raw) To: Dave Wiltshire Cc: davem, netdev, linux-kernel, nsujir, mchan, rmody, jcliburn, chris.snook, jeffrey.t.kirsher, bruce.w.allan, alexander.h.duyck, cooldavid, linux-driver, linux-wimax, wimax, linux-wireless, viro, eparis, edumazet, dev, jhs On Wed, 2013-06-12 at 19:05 +1000, Dave Wiltshire wrote: > Some call sites to pskb_expand_head subsequently update the skb truesize > and others don't (even with non-zero arguments). This is likely a memory > audit leak. Fixed this up by moving the memory accounting to the > skbuff.c file and removing it from the calling sites. > > Signed-off-by: Dave Wiltshire <david.wiltshire@gmx.com> > --- > drivers/net/wireless/mwl8k.c | 1 - > kernel/audit.c | 2 -- > net/core/skbuff.c | 1 + > net/netlink/af_netlink.c | 3 +-- > net/wireless/util.c | 2 -- > 5 files changed, 2 insertions(+), 7 deletions(-) Ouch. Sorry, you cannot do that. skb->truesize is really complex, because there is a strong relation between skb->truesize and memory accounting on sockets. So pskb_expand_head() should not touch skb->truesize. Only callers can do that when needed, and if possible. An example of very careful truesize manipulation can be found in tcp_tso_segment() ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-06-12 9:17 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-12 9:05 [PATCH 0/3] skbuff: pskb_expand_head changes Dave Wiltshire 2013-06-12 9:05 ` [PATCH 1/3] skbuff: Update truesize in pskb_expand_head Dave Wiltshire 2013-06-12 9:16 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox