* Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
From: Pravin Shelar @ 2014-11-21 18:08 UTC (permalink / raw)
To: Joe Stringer; +Cc: dev@openvswitch.org, netdev
In-Reply-To: <CANr6G5zoe=vAme08cHzdM76iYCjMYPz-mcbw236mk1-dVqxLdQ@mail.gmail.com>
On Thu, Nov 20, 2014 at 4:33 PM, Joe Stringer <joestringer@nicira.com> wrote:
> On 19 November 2014 15:34, Pravin Shelar <pshelar@nicira.com> wrote:
>> On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@nicira.com> wrote:
>>> @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
>>>
>>> /* Called with ovs_mutex or RCU read lock. */
>>> static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
>>> - struct sk_buff *skb)
>>> + struct sk_buff *skb, u32 ufid_flags)
>>> {
>>> struct nlattr *nla;
>>> int err;
>>>
>>> - /* Fill flow key. */
>>> - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
>>> - if (!nla)
>>> - return -EMSGSIZE;
>>> -
>>> - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key, skb,
>>> - false);
>>> - if (err)
>>> - return err;
>>> -
>>> - nla_nest_end(skb, nla);
>>> + /* Fill flow key. If userspace didn't specify a UFID, then ignore the
>>> + * OMIT_KEY flag. */
>>> + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
>>> + !flow->index_by_ufid) {
>>
>> I am not sure about this check, userspace needs to send atleast ufid
>> or the unmasked key as id for flow. otherwise we shld flag error. Here
>> we can serialize flow->key.
>> There could be another function which takes care of flow-id
>> serialization where we serialize use ufid or unmasked key as flow id.
>> Lets group ufid and unmasked key together rather than masked key and
>> unmasked key which are not related.
>
> Let me start from scratch and see if this is what you're saying.
>
> fill_id() would serialize the UFID or unmasked key if UFID is not available.
> fill_key() would serialize the masked key if !(ufid_flags &
> OVS_UFID_F_OMIT_KEY) and UFID was provided (but not when the UFID is
> missing).
> fill_mask() would serialize the mask if !(ufid_flags & OVS_UFID_F_OMIT_MASK).
>
> I see key and id as related, because the flow_key serves as both the
> "match" and the "id" in the non-ufid case, so we need to know the same
> piece of information in both functions to ensure we don't serialize
> the key twice.
Lets follow new model where ID and key are two separate attributes. I
agree we also need to check for ufid in key serialization function to
handle compatibility code. Keeping masked and unmasked key separate is
easier to understand.
^ permalink raw reply
* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
From: Jiri Pirko @ 2014-11-21 18:05 UTC (permalink / raw)
To: Pravin Shelar
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <CALnjE+pM=D=OEnkuC3h8iQHzF3Qc33CNA7CMgB2=H0hMboxiKQ@mail.gmail.com>
Fri, Nov 21, 2014 at 05:11:39PM CET, pshelar@nicira.com wrote:
>On Wed, Nov 19, 2014 at 5:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> So it can be used from out of openvswitch code.
>> Did couple of cosmetic changes on the way, namely variable naming and
>> adding support for 8021AD proto.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>
>> v3->v4:
>> - don't try to be smart and copy skb->mac_len manipulation as it is. It can be
>> adjusted later on.
>
>I am fine with moving this code to generic library, as long as we can
>adjust the mac_len as offset for MPLS header, otherwise this breaks
>MPLS support.
Pravin, I'm just moving the code without changing it in this aspect.
Maybe I'm missing something but can you tell me what you see wrong?
Thanks.
>
>> - fix error path in do_execute_actions as suggested by Pravin
>> v2->v3:
>> - used previously introduced __vlan_insert_tag helper
>> - used skb_push/pull to get skb->data into needed point
>> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
>> v1->v2:
>> - adjusted to fix recent ovs changes
>> - included change to use make_writable suggested by Eric
>>
>> include/linux/skbuff.h | 2 +
>> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>> net/openvswitch/actions.c | 86 +++++-------------------------------------
>> 3 files changed, 106 insertions(+), 77 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index e045516..78c299f 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>> int skb_ensure_writable(struct sk_buff *skb, int write_len);
>> +int skb_vlan_pop(struct sk_buff *skb);
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>
>> struct skb_checksum_ops {
>> __wsum (*update)(const void *mem, int len, __wsum wsum);
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index d11bbe0..c906c5f 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
>> }
>> EXPORT_SYMBOL(skb_ensure_writable);
>>
>> +/* remove VLAN header from packet and update csum accordingly. */
>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>> +{
>> + struct vlan_hdr *vhdr;
>> + unsigned int offset = skb->data - skb_mac_header(skb);
>> + int err;
>> +
>> + __skb_push(skb, offset);
>> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>> + if (unlikely(err))
>> + goto pull;
>> +
>> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>> +
>> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
>> +
>> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> + __skb_pull(skb, VLAN_HLEN);
>> +
>> + vlan_set_encap_proto(skb, vhdr);
>> + skb->mac_header += VLAN_HLEN;
>> +
>> + if (skb_network_offset(skb) < ETH_HLEN)
>> + skb_set_network_header(skb, ETH_HLEN);
>> +
>> + skb_reset_mac_len(skb);
>> +pull:
>> + __skb_pull(skb, offset);
>> +
>> + return err;
>> +}
>> +
>> +int skb_vlan_pop(struct sk_buff *skb)
>> +{
>> + u16 vlan_tci;
>> + __be16 vlan_proto;
>> + int err;
>> +
>> + if (likely(vlan_tx_tag_present(skb))) {
>> + skb->vlan_tci = 0;
>> + } else {
>> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
>> + skb->protocol != htons(ETH_P_8021AD)) ||
>> + skb->len < VLAN_ETH_HLEN))
>> + return 0;
>> +
>> + err = __skb_vlan_pop(skb, &vlan_tci);
>> + if (err)
>> + return err;
>> + }
>> + /* move next vlan tag to hw accel tag */
>> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
>> + skb->protocol != htons(ETH_P_8021AD)) ||
>> + skb->len < VLAN_ETH_HLEN))
>> + return 0;
>> +
>> + vlan_proto = skb->protocol;
>> + err = __skb_vlan_pop(skb, &vlan_tci);
>> + if (unlikely(err))
>> + return err;
>> +
>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(skb_vlan_pop);
>> +
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>> +{
>> + if (vlan_tx_tag_present(skb)) {
>> + unsigned int offset = skb->data - skb_mac_header(skb);
>> + int err;
>> +
>> + /* __vlan_insert_tag expect skb->data pointing to mac header.
>> + * So change skb->data before calling it and change back to
>> + * original position later
>> + */
>> + __skb_push(skb, offset);
>> + err = __vlan_insert_tag(skb, skb->vlan_proto,
>> + vlan_tx_tag_get(skb));
>> + if (err)
>> + return err;
>> + skb->protocol = skb->vlan_proto;
>> + skb->mac_len += VLAN_HLEN;
>> + __skb_pull(skb, offset);
>> +
>> + if (skb->ip_summed == CHECKSUM_COMPLETE)
>> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
>> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
>> + }
>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(skb_vlan_push);
>> +
>> /**
>> * alloc_skb_with_frags - allocate skb with page frags
>> *
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 7ffa377..4e05ea1 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
>> return 0;
>> }
>>
>> -/* remove VLAN header from packet and update csum accordingly. */
>> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
>> -{
>> - struct vlan_hdr *vhdr;
>> - int err;
>> -
>> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>> - if (unlikely(err))
>> - return err;
>> -
>> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>> -
>> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> - *current_tci = vhdr->h_vlan_TCI;
>> -
>> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> - __skb_pull(skb, VLAN_HLEN);
>> -
>> - vlan_set_encap_proto(skb, vhdr);
>> - skb->mac_header += VLAN_HLEN;
>> -
>> - if (skb_network_offset(skb) < ETH_HLEN)
>> - skb_set_network_header(skb, ETH_HLEN);
>> -
>> - /* Update mac_len for subsequent MPLS actions */
>> - skb_reset_mac_len(skb);
>> - return 0;
>> -}
>> -
>> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>> {
>> - __be16 tci;
>> int err;
>>
>> - if (likely(vlan_tx_tag_present(skb))) {
>> - skb->vlan_tci = 0;
>> - } else {
>> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
>> - skb->len < VLAN_ETH_HLEN))
>> - return 0;
>> -
>> - err = __pop_vlan_tci(skb, &tci);
>> - if (err)
>> - return err;
>> - }
>> - /* move next vlan tag to hw accel tag */
>> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
>> - skb->len < VLAN_ETH_HLEN)) {
>> + err = skb_vlan_pop(skb);
>> + if (vlan_tx_tag_present(skb))
>> + invalidate_flow_key(key);
>> + else
>> key->eth.tci = 0;
>> - return 0;
>> - }
>> -
>> - invalidate_flow_key(key);
>> - err = __pop_vlan_tci(skb, &tci);
>> - if (unlikely(err))
>> - return err;
>> -
>> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
>> - return 0;
>> + return err;
>> }
>>
>> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
>> const struct ovs_action_push_vlan *vlan)
>> {
>> - if (unlikely(vlan_tx_tag_present(skb))) {
>> - u16 current_tag;
>> -
>> - /* push down current VLAN tag */
>> - current_tag = vlan_tx_tag_get(skb);
>> -
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - current_tag);
>> - if (!skb)
>> - return -ENOMEM;
>> - /* Update mac_len for subsequent MPLS actions */
>> - skb->mac_len += VLAN_HLEN;
>> -
>> - if (skb->ip_summed == CHECKSUM_COMPLETE)
>> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
>> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
>> -
>> + if (vlan_tx_tag_present(skb))
>> invalidate_flow_key(key);
>> - } else {
>> + else
>> key->eth.tci = vlan->vlan_tci;
>> - }
>> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>> - return 0;
>> + return skb_vlan_push(skb, vlan->vlan_tpid,
>> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>> }
>>
>> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
>> @@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>
>> case OVS_ACTION_ATTR_PUSH_VLAN:
>> err = push_vlan(skb, key, nla_data(a));
>> - if (unlikely(err)) /* skb already freed. */
>> - return err;
>> break;
>>
>> case OVS_ACTION_ATTR_POP_VLAN:
>> --
>> 1.9.3
>>
^ permalink raw reply
* Re: [PATCH net] net: Revert "net: avoid one atomic operation in skb_clone()"
From: Sabrina Dubroca @ 2014-11-21 18:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Chris Mason, David Miller, netdev
In-Reply-To: <1416589451.8629.119.camel@edumazet-glaptop2.roam.corp.google.com>
Hello Eric,
2014-11-21, 09:04:11 -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Not sure what I was thinking, but doing anything after
> releasing a refcount is suicidal or/and embarrassing.
>
> By the time we set skb->fclone to SKB_FCLONE_FREE, another cpu
> could have released last reference and freed whole skb.
>
> We potentially corrupt memory or trap if CONFIG_DEBUG_PAGEALLOC is set.
>
> Reported-by: Chris Mason <clm@fb.com>
> Fixes: ce1a4ea3f1258 ("net: avoid one atomic operation in skb_clone()")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> net/core/skbuff.c | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index c16615bfb61e..be4c7deed971 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -552,20 +552,13 @@ static void kfree_skbmem(struct sk_buff *skb)
> case SKB_FCLONE_CLONE:
> fclones = container_of(skb, struct sk_buff_fclones, skb2);
>
> - /* Warning : We must perform the atomic_dec_and_test() before
> - * setting skb->fclone back to SKB_FCLONE_FREE, otherwise
> - * skb_clone() could set clone_ref to 2 before our decrement.
> - * Anyway, if we are going to free the structure, no need to
> - * rewrite skb->fclone.
> + /* The clone portion is available for
> + * fast-cloning again.
> */
> - if (atomic_dec_and_test(&fclones->fclone_ref)) {
> + skb->fclone = SKB_FCLONE_UNAVAILABLE;
Shouldn't that be SKB_FCLONE_FREE ?
> +
> + if (atomic_dec_and_test(&fclones->fclone_ref))
> kmem_cache_free(skbuff_fclone_cache, fclones);
> - } else {
> - /* The clone portion is available for
> - * fast-cloning again.
> - */
> - skb->fclone = SKB_FCLONE_FREE;
like here ^^^^
Thanks,
--
Sabrina
^ permalink raw reply
* Charity Work
From: luv2charitys @ 2014-11-21 17:54 UTC (permalink / raw)
To: netdev
Hello,this is Mr Paul N,i sent you an email on charity work but i am yet to hear fom you,do reply with this code CHA-2015 to my email address paulcharity@qq.com i Look forward to hearing from you this time,God bless Brother Paul
^ permalink raw reply
* Re: [PATCHv2 net] i40e: Implement ndo_gso_check()
From: Joe Stringer @ 2014-11-21 17:59 UTC (permalink / raw)
To: Jesse Gross
Cc: netdev, Shannon Nelson, Brandeburg, Jesse, Jeff Kirsher,
Tom Herbert, linux.nics, Linux Kernel Mailing List
In-Reply-To: <CAEP_g=-VEWi2Qo6gcJGr2+NXWEhkuMyu7BiAke6H=x+PTLcNcg@mail.gmail.com>
On 20 November 2014 16:19, Jesse Gross <jesse@nicira.com> wrote:
> On Thu, Nov 20, 2014 at 3:11 PM, Joe Stringer <joestringer@nicira.com> wrote:
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index c3a7f4a..2b01c8d 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> @@ -7447,6 +7447,36 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb,
>>
>> #endif /* USE_DEFAULT_FDB_DEL_DUMP */
>> #endif /* HAVE_FDB_OPS */
>> +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + if ((skb_shinfo(skb)->gso_type & SKB_GSO_IPIP) &&
>> + (skb->inner_protocol_type != ENCAP_TYPE_IPPROTO ||
>> + skb->inner_protocol != htons(IPPROTO_IPIP))) {
>
> I think this check on inner_protocol isn't really semantically correct
> - that field is a union with inner_ipproto, so it would be more
> correct to check against that and then you wouldn't need to use htons
> either.
Yes, translation error on my part, but if IPIP isn't exposed I'll just
drop this section.
> I don't know if we need to have the check at all for IPIP though -
> after all the driver doesn't expose support for it all (actually it
> doesn't expose GRE either). This raises kind of an interesting
> question about the checks though - it's pretty easy to add support to
> the driver for a new GSO type (and I imagine that people will be
> adding GRE soon) and forget to update the check.
If the check is more conservative, then testing would show that it's
not working and lead people to figure out why (and update the check).
>> + if (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE | SKB_GSO_UDP_TUNNEL)) {
>> + unsigned char *ihdr;
>> +
>> + if (skb->inner_protocol_type != ENCAP_TYPE_ETHER)
>> + return false;
>
> I guess if you want to be nice, it might be good to check the
> equivalent IP protocol types as well.
Can do (just UDP as I'll drop GRE as per the above).
^ permalink raw reply
* RE: [RFC] situation with csum_and_copy_... API
From: David Laight @ 2014-11-21 17:42 UTC (permalink / raw)
To: 'Al Viro', Eric Dumazet
Cc: David Miller, torvalds@linux-foundation.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
target-devel@vger.kernel.org, Nicholas A. Bellinger,
Christoph Hellwig
In-Reply-To: <20141121084956.GT7996@ZenIV.linux.org.uk>
From: Al Viro
...
> We would be better off with iov_iter passed to __sock_{send,recv}msg() (as
> a part of struct msghdr, instead of ->msg_iov/->msg_iovlen) and always
> advanced to match the amount of data actually picked from it. With iovec
> behind it remaining constant. That would work just as well as the current
> variant for sendmsg(2)/recvmsg(2)/etc., be a lot more convenient for
> kernel_{send,recv}msg() callers and would allow a lot of other fun stuff.
Callers of kernel_send/recvmsg() could easily be using a wrapper
function that creates the 'msghdr'.
When the want to send the remaining part of a buffer the old iterator
will no longer be available - just the original iov and the required offset.
So it would be useful if the iterator could be initialised to a byte
offset down the iov[].
Are there any current code paths where the iov[] is modified but
ends up being something other than 'the remaining data'?
If not then code can check whether iov[0].len has changed, and
skip the 'advance' is it has.
(I've an out-of-tree driver that assumes the iov[] isn't changed.)
David
^ permalink raw reply
* Re: [RFC] situation with csum_and_copy_... API
From: David Miller @ 2014-11-21 17:26 UTC (permalink / raw)
To: viro; +Cc: torvalds, netdev, linux-kernel, target-devel, nab, hch
In-Reply-To: <20141120.182339.972861702759954603.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Thu, 20 Nov 2014 18:23:39 -0500 (EST)
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Thu, 20 Nov 2014 21:47:53 +0000
>
>> On Wed, Nov 19, 2014 at 04:53:40PM -0500, David Miller wrote:
>>
>>> Pulled, thanks Al.
>>
>> Umm... Not in net-next.git#master...
>
> Sorry, I may have done my usual: pull at the office compute, fire up
> build testing, go home without pushing back out to kernel.org
>
> :-/
>
> I'll check first thing tomorrow morning.
I've resolved this now, sorry for the inconvenience Al.
^ permalink raw reply
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Chris Mason @ 2014-11-21 17:18 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1416588995.8629.114.camel@edumazet-glaptop2.roam.corp.google.com>
On Fri, Nov 21, 2014 at 11:56 AM, Eric Dumazet <eric.dumazet@gmail.com>
wrote:
> On Fri, 2014-11-21 at 11:47 -0500, Chris Mason wrote:
>
>> Once my current run is done, I can revert this and try a loop. But
>> I
>> don't see how this commit causes a use-after-free on the skb itself?
>
> Well, it is so obvious, you really want to make me miserable ;)
>
> I am testing the revert and send it asap.
Grin, no I just don't know what fclones really are ;) So dropping the
ref by one on the clone opens up the skb to be freed and another CPU
races in and frees skb before we can do the assignment lower down.
I've added your revert here and I'll restart the test. It takes a few
hours to get to the point where I've fallen over so far today, but I'll
just leave it in a loop all day long.
Thanks!
-chris
^ permalink raw reply
* [PATCH net] net: Revert "net: avoid one atomic operation in skb_clone()"
From: Eric Dumazet @ 2014-11-21 17:04 UTC (permalink / raw)
To: Chris Mason, David Miller; +Cc: netdev
In-Reply-To: <1416587469.8629.106.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
Not sure what I was thinking, but doing anything after
releasing a refcount is suicidal or/and embarrassing.
By the time we set skb->fclone to SKB_FCLONE_FREE, another cpu
could have released last reference and freed whole skb.
We potentially corrupt memory or trap if CONFIG_DEBUG_PAGEALLOC is set.
Reported-by: Chris Mason <clm@fb.com>
Fixes: ce1a4ea3f1258 ("net: avoid one atomic operation in skb_clone()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/core/skbuff.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c16615bfb61e..be4c7deed971 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -552,20 +552,13 @@ static void kfree_skbmem(struct sk_buff *skb)
case SKB_FCLONE_CLONE:
fclones = container_of(skb, struct sk_buff_fclones, skb2);
- /* Warning : We must perform the atomic_dec_and_test() before
- * setting skb->fclone back to SKB_FCLONE_FREE, otherwise
- * skb_clone() could set clone_ref to 2 before our decrement.
- * Anyway, if we are going to free the structure, no need to
- * rewrite skb->fclone.
+ /* The clone portion is available for
+ * fast-cloning again.
*/
- if (atomic_dec_and_test(&fclones->fclone_ref)) {
+ skb->fclone = SKB_FCLONE_UNAVAILABLE;
+
+ if (atomic_dec_and_test(&fclones->fclone_ref))
kmem_cache_free(skbuff_fclone_cache, fclones);
- } else {
- /* The clone portion is available for
- * fast-cloning again.
- */
- skb->fclone = SKB_FCLONE_FREE;
- }
break;
}
}
@@ -887,11 +880,7 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
if (skb->fclone == SKB_FCLONE_ORIG &&
n->fclone == SKB_FCLONE_FREE) {
n->fclone = SKB_FCLONE_CLONE;
- /* As our fastclone was free, clone_ref must be 1 at this point.
- * We could use atomic_inc() here, but it is faster
- * to set the final value.
- */
- atomic_set(&fclones->fclone_ref, 2);
+ atomic_inc(&fclones->fclone_ref);
} else {
if (skb_pfmemalloc(skb))
gfp_mask |= __GFP_MEMALLOC;
^ permalink raw reply related
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Chris Mason @ 2014-11-21 16:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1416587469.8629.106.camel@edumazet-glaptop2.roam.corp.google.com>
On Fri, Nov 21, 2014 at 11:31 AM, Eric Dumazet <eric.dumazet@gmail.com>
wrote:
> On Fri, 2014-11-21 at 11:16 -0500, Chris Mason wrote:
>> Hi everyone,
>>
>> I've hit this a few times today while hammering on my btrfs queue
>> for
>> the next merge window. It's plain v3.18-rc5 plus a few btrfs
>> patches,
>> so it isn't impossible a btrfs double free is causing trouble.
>>
>> But, that should also show up in places outside the networking
>> stack and I've
>> gotten this exact stack trace twice now:
>>
>> [ 2255.152925] BUG: unable to handle kernel paging request at
>> ffff880fa1f91f96
>> [ 2255.185251] [<ffffffff81595f68>] __kfree_skb+0x58/0xc0Hi Chris
>
> Can you double check, or send whole __kfree_skb() disassembly ?
>
> I do not understand how skb->fclone could possibly trap _at_ this
> point.
So I double checked and got worried about the orb instruction until I
realized fclone is in a bitfield:
Dump of assembler code for function __kfree_skb:
0xffffffff81595f10 <+0>: push %rbp
0xffffffff81595f11 <+1>: mov %rsp,%rbp
0xffffffff81595f14 <+4>: push %rbx
0xffffffff81595f15 <+5>: sub $0x8,%rsp
0xffffffff81595f19 <+9>: callq 0xffffffff81672c40 <mcount>
0xffffffff81595f1e <+14>: mov %rdi,%rbx
0xffffffff81595f21 <+17>: callq 0xffffffff81595ee0 <skb_release_all>
0xffffffff81595f26 <+22>: movzbl 0x7e(%rbx),%eax
0xffffffff81595f2a <+26>: shr $0x2,%al
0xffffffff81595f2d <+29>: and $0x3,%eax
0xffffffff81595f30 <+32>: cmp $0x1,%eax
0xffffffff81595f33 <+35>: je 0xffffffff81595f78 <__kfree_skb+104>
0xffffffff81595f35 <+37>: cmp $0x2,%eax
0xffffffff81595f38 <+40>: je 0xffffffff81595f58 <__kfree_skb+72>
0xffffffff81595f3a <+42>: test %eax,%eax
0xffffffff81595f3c <+44>: jne 0xffffffff81595f4d <__kfree_skb+61>
0xffffffff81595f3e <+46>: mov %rbx,%rsi
0xffffffff81595f41 <+49>: mov 0x760858(%rip),%rdi #
0xffffffff81cf67a0 <skbuff_head_cache>
0xffffffff81595f48 <+56>: callq 0xffffffff81190580 <kmem_cache_free>
0xffffffff81595f4d <+61>: add $0x8,%rsp
0xffffffff81595f51 <+65>: pop %rbx
0xffffffff81595f52 <+66>: leaveq
0xffffffff81595f53 <+67>: retq
0xffffffff81595f54 <+68>: nopl 0x0(%rax)
0xffffffff81595f58 <+72>: lea -0xd8(%rbx),%rsi
0xffffffff81595f5f <+79>: lock decl 0x1b0(%rsi)
0xffffffff81595f66 <+86>: je 0xffffffff81595fb0 <__kfree_skb+160>
0xffffffff81595f68 <+88>: orb $0xc,0x7e(%rbx)
^^^^^^^^^^^^^^^^^^^^^
Should be skb->fclone = SKB_FCLONE_FREE;
0xffffffff81595f6c <+92>: add $0x8,%rsp
0xffffffff81595f70 <+96>: pop %rbx
0xffffffff81595f71 <+97>: leaveq
0xffffffff81595f72 <+98>: retq
0xffffffff81595f73 <+99>: nopl 0x0(%rax,%rax,1)
0xffffffff81595f78 <+104>: lock decl 0x1b0(%rbx)
0xffffffff81595f7f <+111>: je 0xffffffff81595f90
<__kfree_skb+128>
0xffffffff81595f81 <+113>: add $0x8,%rsp
0xffffffff81595f85 <+117>: pop %rbx
0xffffffff81595f86 <+118>: leaveq
0xffffffff81595f87 <+119>: retq
0xffffffff81595f88 <+120>: nopl 0x0(%rax,%rax,1)
0xffffffff81595f90 <+128>: mov %rbx,%rsi
0xffffffff81595f93 <+131>: mov 0x7607fe(%rip),%rdi #
0xffffffff81cf6798 <skbuff_fclone_cache>
0xffffffff81595f9a <+138>: callq 0xffffffff81190580
<kmem_cache_free>
0xffffffff81595f9f <+143>: add $0x8,%rsp
0xffffffff81595fa3 <+147>: pop %rbx
0xffffffff81595fa4 <+148>: leaveq
0xffffffff81595fa5 <+149>: retq
0xffffffff81595fa6 <+150>: nopw %cs:0x0(%rax,%rax,1)
0xffffffff81595fb0 <+160>: mov 0x7607e1(%rip),%rdi #
0xffffffff81cf6798 <skbuff_fclone_cache>
0xffffffff81595fb7 <+167>: callq 0xffffffff81190580
<kmem_cache_free>
0xffffffff81595fbc <+172>: add $0x8,%rsp
0xffffffff81595fc0 <+176>: pop %rbx
0xffffffff81595fc1 <+177>: leaveq
0xffffffff81595fc2 <+178>: retq
-chris
^ permalink raw reply
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Eric Dumazet @ 2014-11-21 16:56 UTC (permalink / raw)
To: Chris Mason; +Cc: netdev
In-Reply-To: <1416588426.24312.6@mail.thefacebook.com>
On Fri, 2014-11-21 at 11:47 -0500, Chris Mason wrote:
> Once my current run is done, I can revert this and try a loop. But I
> don't see how this commit causes a use-after-free on the skb itself?
Well, it is so obvious, you really want to make me miserable ;)
I am testing the revert and send it asap.
^ permalink raw reply
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Chris Mason @ 2014-11-21 16:47 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1416587848.8629.108.camel@edumazet-glaptop2.roam.corp.google.com>
On Fri, Nov 21, 2014 at 11:37 AM, Eric Dumazet <eric.dumazet@gmail.com>
wrote:
> On Fri, 2014-11-21 at 08:31 -0800, Eric Dumazet wrote:
>
>> Can you double check, or send whole __kfree_skb() disassembly ?
>>
>> I do not understand how skb->fclone could possibly trap _at_ this
>> point.
>
> Oh well, I think commit ce1a4ea3f1258 ("net: avoid one atomic
> operation
> in skb_clone()") is the problem, I'll send a revert.
Once my current run is done, I can revert this and try a loop. But I
don't see how this commit causes a use-after-free on the skb itself?
-chris
^ permalink raw reply
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Chris Mason @ 2014-11-21 16:41 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1416587469.8629.106.camel@edumazet-glaptop2.roam.corp.google.com>
On Fri, Nov 21, 2014 at 11:31 AM, Eric Dumazet <eric.dumazet@gmail.com>
wrote:
> On Fri, 2014-11-21 at 11:16 -0500, Chris Mason wrote:
>> Hi everyone,
>>
>> I've hit this a few times today while hammering on my btrfs queue
>> for
>> the next merge window. It's plain v3.18-rc5 plus a few btrfs
>> patches,
>> so it isn't impossible a btrfs double free is causing trouble.
>>
>> But, that should also show up in places outside the networking
>> stack and I've
>> gotten this exact stack trace twice now:
>>
>> [ 2255.152925] BUG: unable to handle kernel paging request at
>> ffff880fa1f91f96
>> [ 2255.185251] [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
>> [ 2255.196223] PGD 2be4067 PUD 10783cb067 PMD 10782bb067 PTE
>> 8000000fa1f91060
>> [ 2255.210163] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
>> [ 2255.219394] Modules linked in: btrfs raid6_pq zlib_deflate
>> lzo_compress xor xfs exportfs libcrc32c nfsv4 fuse k10temp coretemp
>> hwmon tcp_diag inet_diag loop ip6table_filter ip6_tables xt_NFLOG
>> nfnetlink_log nfnetlink xt_comment xt_statistic iptable_filter
>> ip_tables x_tables nfsv3 nfs lockd grace mptctl netconsole autofs4
>> rpcsec_gss_krb5 auth_rpcgss oid_registry sunrpc ipv6 ext3 jbd dm_mod
>> iTCO_wdt iTCO_vendor_support rtc_cmos ipmi_si ipmi_msghandler pcspkr
>> i2c_i801 lpc_ich mfd_core shpchp ehci_pci ehci_hcd mlx4_en ptp
>> pps_core mlx4_core ses enclosure sg button megaraid_sas
>> [ 2255.323468] CPU: 14 PID: 8517 Comm: scribe-event Not tainted
>> 3.18.0-rc5-mason+ #62
>> [ 2255.338754] Hardware name: ZTSYSTEMS Echo Ridge T4 /A9DRPF-10D,
>> BIOS 1.07 05/10/2012
>> [ 2255.354557] task: ffff881018b61d10 ti: ffff880ff6ae4000 task.ti:
>> ffff880ff6ae4000
>> [ 2255.369680] RIP: 0010:[<ffffffff81595f68>] [<ffffffff81595f68>]
>> __kfree_skb+0x58/0xc0
>> [ 2255.385709] RSP: 0018:ffff880ff6ae7b98 EFLAGS: 00010202
>> [ 2255.396398] RAX: 0000000000000002 RBX: ffff880fa1f91f18 RCX:
>> ffffffff81cd5d80
>> [ 2255.410728] RDX: 00000000ffffffff RSI: ffff880fa1f91e40 RDI:
>> ffff880fa1f91f18
>> [ 2255.425062] RBP: ffff880ff6ae7ba8 R08: 000000000000001b R09:
>> 0000000000000000
>> [ 2255.439379] R10: ffff8810385ef640 R11: ffff8810385ef758 R12:
>> 0000000000000000
>> [ 2255.453702] R13: ffff880fa1f91f40 R14: 0000000000000000 R15:
>> ffff8810385efd4c
>> [ 2255.468024] FS: 00007ff18ebff700(0000)
>> GS:ffff881077cc0000(0000) knlGS:0000000000000000
>> [ 2255.484321] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 2255.495864] CR2: ffff880fa1f91f96 CR3: 0000000850174000 CR4:
>> 00000000000407e0
>> [ 2255.510188] Stack:
>> [ 2255.514279] 0000000000000000 ffff880fa1f91f18 ffff880ff6ae7ca8
>> ffffffff815f27aa
>> [ 2255.529306] ffff881018b61d10 0000000000000001 000000000000001b
>> ffff8810385ef640
>> [ 2255.544337] ffff8810385ef758 ffff8810385ef7a8 ffff881018b61d10
>> 0000000000000000
>> [ 2255.559369] Call Trace:
>> [ 2255.564332] [<ffffffff815f27aa>] tcp_recvmsg+0xa2a/0xd10
>> [ 2255.575198] [<ffffffff8161dcb1>] inet_recvmsg+0xe1/0x110
>> [ 2255.586056] [<ffffffff8158c573>] sock_recvmsg+0xa3/0xd0
>> [ 2255.596740] [<ffffffff811c9e65>] ? __fget_light+0x25/0x60
>> [ 2255.607768] [<ffffffff8158c664>] SYSC_recvfrom+0xc4/0x130
>> [ 2255.618801] [<ffffffff810e987c>] ?
>> __audit_syscall_entry+0xac/0x110
>> [ 2255.631566] [<ffffffff810b6635>] ? current_kernel_time+0x95/0xb0
>> [ 2255.643826] [<ffffffff8109537d>] ?
>> trace_hardirqs_on_caller+0xfd/0x1c0
>> [ 2255.657122] [<ffffffff8158c6de>] SyS_recvfrom+0xe/0x10
>> [ 2255.667632] [<ffffffff81670b92>] system_call_fastpath+0x12/0x17
>> [ 2255.679699] Code: 0f 48 89 de 48 8b 3d 58 08 76 00 e8 33 a6 bf
>> ff 48 83 c4 08 5b c9 c3 0f 1f 40 00 48 8d b3 28 ff ff ff f0 ff 8e b0
>> 01 00 00 74 48 <80> 4b 7e 0c 48 83 c4 08 5b c9 c3 0f 1f 44 00 00 f0
>> ff 8b b0 01
>> [ 2255.719771] RIP [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
>> [ 2255.731019] RSP <ffff880ff6ae7b98>
>> [ 2255.738081] CR2: ffff880fa1f91f96
>> [ 2255.745371] ---[ end trace 982fb6dd92d9b65b ]---
>>
>> Which translates to:
>>
>> 0xffffffff81595f68 is in __kfree_skb (net/core/skbuff.c:567).
>> 562 kmem_cache_free(skbuff_fclone_cache, fclones);
>> 563 } else {
>> 564 /* The clone portion is available for
>> 565 * fast-cloning again.
>> 566 */
>> 567 skb->fclone = SKB_FCLONE_FREE;
>> 568 }
>> 569 break;
>> 570 }
>> 571 }
>>
>> Just looking for related code in the changelog, this one might be
>> related:
>>
>> commit c8753d55afb436fd6a25c8bbe8d783f6dcf1c9f8
>> Author: Vijay Subramanian <subramanian.vijay@gmail.com>
>> Date: Thu Oct 2 10:00:43 2014 -0700
>>
>> net: Cleanup skb cloning by adding SKB_FCLONE_FREE
>>
>> I'm not hitting this consistently enough for a revert or a bisect to
>> prove anything.
>
> Hi Chris
>
> Can you double check, or send whole __kfree_skb() disassembly ?
>
> I do not understand how skb->fclone could possibly trap _at_ this
> point.
I'm running with CONFIG_DEBUG_PAGEALLOC, so skb is in a page that has
been freed. We're crashing just because we touched it.
-chris
^ permalink raw reply
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Eric Dumazet @ 2014-11-21 16:37 UTC (permalink / raw)
To: Chris Mason; +Cc: netdev
In-Reply-To: <1416587469.8629.106.camel@edumazet-glaptop2.roam.corp.google.com>
On Fri, 2014-11-21 at 08:31 -0800, Eric Dumazet wrote:
> Can you double check, or send whole __kfree_skb() disassembly ?
>
> I do not understand how skb->fclone could possibly trap _at_ this point.
Oh well, I think commit ce1a4ea3f1258 ("net: avoid one atomic operation
in skb_clone()") is the problem, I'll send a revert.
^ permalink raw reply
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Chris Mason @ 2014-11-21 16:33 UTC (permalink / raw)
To: netdev
In-Reply-To: <20141121160937.GA32608@ret.masoncoding.com>
On Fri, Nov 21, 2014 at 11:16 AM, Chris Mason <clm@fb.com> wrote:
> Hi everyone,
>
> I've hit this a few times today while hammering on my btrfs queue for
> the next merge window. It's plain v3.18-rc5 plus a few btrfs patches,
> so it isn't impossible a btrfs double free is causing trouble.
Looking through my console logs, I also hit it on v3.18-rc3 and on a
test in the middle of the 3.18 merge window (don't have the exact sha,
sorry).
-chris
^ permalink raw reply
* Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Eric Dumazet @ 2014-11-21 16:31 UTC (permalink / raw)
To: Chris Mason; +Cc: netdev
In-Reply-To: <20141121160937.GA32608@ret.masoncoding.com>
On Fri, 2014-11-21 at 11:16 -0500, Chris Mason wrote:
> Hi everyone,
>
> I've hit this a few times today while hammering on my btrfs queue for
> the next merge window. It's plain v3.18-rc5 plus a few btrfs patches,
> so it isn't impossible a btrfs double free is causing trouble.
>
> But, that should also show up in places outside the networking stack and I've
> gotten this exact stack trace twice now:
>
> [ 2255.152925] BUG: unable to handle kernel paging request at ffff880fa1f91f96
> [ 2255.185251] [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
> [ 2255.196223] PGD 2be4067 PUD 10783cb067 PMD 10782bb067 PTE 8000000fa1f91060
> [ 2255.210163] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
> [ 2255.219394] Modules linked in: btrfs raid6_pq zlib_deflate lzo_compress xor xfs exportfs libcrc32c nfsv4 fuse k10temp coretemp hwmon tcp_diag inet_diag loop ip6table_filter ip6_tables xt_NFLOG nfnetlink_log nfnetlink xt_comment xt_statistic iptable_filter ip_tables x_tables nfsv3 nfs lockd grace mptctl netconsole autofs4 rpcsec_gss_krb5 auth_rpcgss oid_registry sunrpc ipv6 ext3 jbd dm_mod iTCO_wdt iTCO_vendor_support rtc_cmos ipmi_si ipmi_msghandler pcspkr i2c_i801 lpc_ich mfd_core shpchp ehci_pci ehci_hcd mlx4_en ptp pps_core mlx4_core ses enclosure sg button megaraid_sas
> [ 2255.323468] CPU: 14 PID: 8517 Comm: scribe-event Not tainted 3.18.0-rc5-mason+ #62
> [ 2255.338754] Hardware name: ZTSYSTEMS Echo Ridge T4 /A9DRPF-10D, BIOS 1.07 05/10/2012
> [ 2255.354557] task: ffff881018b61d10 ti: ffff880ff6ae4000 task.ti: ffff880ff6ae4000
> [ 2255.369680] RIP: 0010:[<ffffffff81595f68>] [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
> [ 2255.385709] RSP: 0018:ffff880ff6ae7b98 EFLAGS: 00010202
> [ 2255.396398] RAX: 0000000000000002 RBX: ffff880fa1f91f18 RCX: ffffffff81cd5d80
> [ 2255.410728] RDX: 00000000ffffffff RSI: ffff880fa1f91e40 RDI: ffff880fa1f91f18
> [ 2255.425062] RBP: ffff880ff6ae7ba8 R08: 000000000000001b R09: 0000000000000000
> [ 2255.439379] R10: ffff8810385ef640 R11: ffff8810385ef758 R12: 0000000000000000
> [ 2255.453702] R13: ffff880fa1f91f40 R14: 0000000000000000 R15: ffff8810385efd4c
> [ 2255.468024] FS: 00007ff18ebff700(0000) GS:ffff881077cc0000(0000) knlGS:0000000000000000
> [ 2255.484321] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 2255.495864] CR2: ffff880fa1f91f96 CR3: 0000000850174000 CR4: 00000000000407e0
> [ 2255.510188] Stack:
> [ 2255.514279] 0000000000000000 ffff880fa1f91f18 ffff880ff6ae7ca8 ffffffff815f27aa
> [ 2255.529306] ffff881018b61d10 0000000000000001 000000000000001b ffff8810385ef640
> [ 2255.544337] ffff8810385ef758 ffff8810385ef7a8 ffff881018b61d10 0000000000000000
> [ 2255.559369] Call Trace:
> [ 2255.564332] [<ffffffff815f27aa>] tcp_recvmsg+0xa2a/0xd10
> [ 2255.575198] [<ffffffff8161dcb1>] inet_recvmsg+0xe1/0x110
> [ 2255.586056] [<ffffffff8158c573>] sock_recvmsg+0xa3/0xd0
> [ 2255.596740] [<ffffffff811c9e65>] ? __fget_light+0x25/0x60
> [ 2255.607768] [<ffffffff8158c664>] SYSC_recvfrom+0xc4/0x130
> [ 2255.618801] [<ffffffff810e987c>] ? __audit_syscall_entry+0xac/0x110
> [ 2255.631566] [<ffffffff810b6635>] ? current_kernel_time+0x95/0xb0
> [ 2255.643826] [<ffffffff8109537d>] ? trace_hardirqs_on_caller+0xfd/0x1c0
> [ 2255.657122] [<ffffffff8158c6de>] SyS_recvfrom+0xe/0x10
> [ 2255.667632] [<ffffffff81670b92>] system_call_fastpath+0x12/0x17
> [ 2255.679699] Code: 0f 48 89 de 48 8b 3d 58 08 76 00 e8 33 a6 bf ff 48 83 c4 08 5b c9 c3 0f 1f 40 00 48 8d b3 28 ff ff ff f0 ff 8e b0 01 00 00 74 48 <80> 4b 7e 0c 48 83 c4 08 5b c9 c3 0f 1f 44 00 00 f0 ff 8b b0 01
> [ 2255.719771] RIP [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
> [ 2255.731019] RSP <ffff880ff6ae7b98>
> [ 2255.738081] CR2: ffff880fa1f91f96
> [ 2255.745371] ---[ end trace 982fb6dd92d9b65b ]---
>
> Which translates to:
>
> 0xffffffff81595f68 is in __kfree_skb (net/core/skbuff.c:567).
> 562 kmem_cache_free(skbuff_fclone_cache, fclones);
> 563 } else {
> 564 /* The clone portion is available for
> 565 * fast-cloning again.
> 566 */
> 567 skb->fclone = SKB_FCLONE_FREE;
> 568 }
> 569 break;
> 570 }
> 571 }
>
> Just looking for related code in the changelog, this one might be
> related:
>
> commit c8753d55afb436fd6a25c8bbe8d783f6dcf1c9f8
> Author: Vijay Subramanian <subramanian.vijay@gmail.com>
> Date: Thu Oct 2 10:00:43 2014 -0700
>
> net: Cleanup skb cloning by adding SKB_FCLONE_FREE
>
> I'm not hitting this consistently enough for a revert or a bisect to
> prove anything.
Hi Chris
Can you double check, or send whole __kfree_skb() disassembly ?
I do not understand how skb->fclone could possibly trap _at_ this point.
^ permalink raw reply
* crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC
From: Chris Mason @ 2014-11-21 16:16 UTC (permalink / raw)
To: netdev
Hi everyone,
I've hit this a few times today while hammering on my btrfs queue for
the next merge window. It's plain v3.18-rc5 plus a few btrfs patches,
so it isn't impossible a btrfs double free is causing trouble.
But, that should also show up in places outside the networking stack and I've
gotten this exact stack trace twice now:
[ 2255.152925] BUG: unable to handle kernel paging request at ffff880fa1f91f96
[ 2255.185251] [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
[ 2255.196223] PGD 2be4067 PUD 10783cb067 PMD 10782bb067 PTE 8000000fa1f91060
[ 2255.210163] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
[ 2255.219394] Modules linked in: btrfs raid6_pq zlib_deflate lzo_compress xor xfs exportfs libcrc32c nfsv4 fuse k10temp coretemp hwmon tcp_diag inet_diag loop ip6table_filter ip6_tables xt_NFLOG nfnetlink_log nfnetlink xt_comment xt_statistic iptable_filter ip_tables x_tables nfsv3 nfs lockd grace mptctl netconsole autofs4 rpcsec_gss_krb5 auth_rpcgss oid_registry sunrpc ipv6 ext3 jbd dm_mod iTCO_wdt iTCO_vendor_support rtc_cmos ipmi_si ipmi_msghandler pcspkr i2c_i801 lpc_ich mfd_core shpchp ehci_pci ehci_hcd mlx4_en ptp pps_core mlx4_core ses enclosure sg button megaraid_sas
[ 2255.323468] CPU: 14 PID: 8517 Comm: scribe-event Not tainted 3.18.0-rc5-mason+ #62
[ 2255.338754] Hardware name: ZTSYSTEMS Echo Ridge T4 /A9DRPF-10D, BIOS 1.07 05/10/2012
[ 2255.354557] task: ffff881018b61d10 ti: ffff880ff6ae4000 task.ti: ffff880ff6ae4000
[ 2255.369680] RIP: 0010:[<ffffffff81595f68>] [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
[ 2255.385709] RSP: 0018:ffff880ff6ae7b98 EFLAGS: 00010202
[ 2255.396398] RAX: 0000000000000002 RBX: ffff880fa1f91f18 RCX: ffffffff81cd5d80
[ 2255.410728] RDX: 00000000ffffffff RSI: ffff880fa1f91e40 RDI: ffff880fa1f91f18
[ 2255.425062] RBP: ffff880ff6ae7ba8 R08: 000000000000001b R09: 0000000000000000
[ 2255.439379] R10: ffff8810385ef640 R11: ffff8810385ef758 R12: 0000000000000000
[ 2255.453702] R13: ffff880fa1f91f40 R14: 0000000000000000 R15: ffff8810385efd4c
[ 2255.468024] FS: 00007ff18ebff700(0000) GS:ffff881077cc0000(0000) knlGS:0000000000000000
[ 2255.484321] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2255.495864] CR2: ffff880fa1f91f96 CR3: 0000000850174000 CR4: 00000000000407e0
[ 2255.510188] Stack:
[ 2255.514279] 0000000000000000 ffff880fa1f91f18 ffff880ff6ae7ca8 ffffffff815f27aa
[ 2255.529306] ffff881018b61d10 0000000000000001 000000000000001b ffff8810385ef640
[ 2255.544337] ffff8810385ef758 ffff8810385ef7a8 ffff881018b61d10 0000000000000000
[ 2255.559369] Call Trace:
[ 2255.564332] [<ffffffff815f27aa>] tcp_recvmsg+0xa2a/0xd10
[ 2255.575198] [<ffffffff8161dcb1>] inet_recvmsg+0xe1/0x110
[ 2255.586056] [<ffffffff8158c573>] sock_recvmsg+0xa3/0xd0
[ 2255.596740] [<ffffffff811c9e65>] ? __fget_light+0x25/0x60
[ 2255.607768] [<ffffffff8158c664>] SYSC_recvfrom+0xc4/0x130
[ 2255.618801] [<ffffffff810e987c>] ? __audit_syscall_entry+0xac/0x110
[ 2255.631566] [<ffffffff810b6635>] ? current_kernel_time+0x95/0xb0
[ 2255.643826] [<ffffffff8109537d>] ? trace_hardirqs_on_caller+0xfd/0x1c0
[ 2255.657122] [<ffffffff8158c6de>] SyS_recvfrom+0xe/0x10
[ 2255.667632] [<ffffffff81670b92>] system_call_fastpath+0x12/0x17
[ 2255.679699] Code: 0f 48 89 de 48 8b 3d 58 08 76 00 e8 33 a6 bf ff 48 83 c4 08 5b c9 c3 0f 1f 40 00 48 8d b3 28 ff ff ff f0 ff 8e b0 01 00 00 74 48 <80> 4b 7e 0c 48 83 c4 08 5b c9 c3 0f 1f 44 00 00 f0 ff 8b b0 01
[ 2255.719771] RIP [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
[ 2255.731019] RSP <ffff880ff6ae7b98>
[ 2255.738081] CR2: ffff880fa1f91f96
[ 2255.745371] ---[ end trace 982fb6dd92d9b65b ]---
Which translates to:
0xffffffff81595f68 is in __kfree_skb (net/core/skbuff.c:567).
562 kmem_cache_free(skbuff_fclone_cache, fclones);
563 } else {
564 /* The clone portion is available for
565 * fast-cloning again.
566 */
567 skb->fclone = SKB_FCLONE_FREE;
568 }
569 break;
570 }
571 }
Just looking for related code in the changelog, this one might be
related:
commit c8753d55afb436fd6a25c8bbe8d783f6dcf1c9f8
Author: Vijay Subramanian <subramanian.vijay@gmail.com>
Date: Thu Oct 2 10:00:43 2014 -0700
net: Cleanup skb cloning by adding SKB_FCLONE_FREE
I'm not hitting this consistently enough for a revert or a bisect to
prove anything.
-chris
^ permalink raw reply
* Re: [PATCH v2 9/9] netfilter: Replace smp_read_barrier_depends() with lockless_dereference()
From: Eric Dumazet @ 2014-11-21 16:12 UTC (permalink / raw)
To: Pranith Kumar
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, netfilter-devel, coreteam,
open list:NETWORKING [IPv4/..., open list
In-Reply-To: <1416582363-20661-10-git-send-email-bobby.prani@gmail.com>
On Fri, 2014-11-21 at 10:06 -0500, Pranith Kumar wrote:
> Recently lockless_dereference() was added which can be used in place of
> hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
> net/ipv4/netfilter/arp_tables.c | 3 +--
> net/ipv4/netfilter/ip_tables.c | 3 +--
> net/ipv6/netfilter/ip6_tables.c | 3 +--
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index f95b6f9..fc7533d 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -270,12 +270,11 @@ unsigned int arpt_do_table(struct sk_buff *skb,
>
> local_bh_disable();
> addend = xt_write_recseq_begin();
> - private = table->private;
> /*
> * Ensure we load private-> members after we've fetched the base
> * pointer.
> */
> - smp_read_barrier_depends();
> + private = lockless_dereference(table->private);
> table_base = private->entries[smp_processor_id()];
>
Please carefully read the code, before and after your change, then
you'll see this change broke the code.
Problem is that a bug like that can be really hard to diagnose and fix
later, so really you have to be very careful doing these mechanical
changes.
IMO, current code+comment is better than with this
lockless_dereference() which in this particular case obfuscates the
code. more than anything.
In this case we do have a lock (sort of), so lockless_dereference() is
quite misleading.
^ permalink raw reply
* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
From: Pravin Shelar @ 2014-11-21 16:11 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <1416402303-25341-9-git-send-email-jiri@resnulli.us>
On Wed, Nov 19, 2014 at 5:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> So it can be used from out of openvswitch code.
> Did couple of cosmetic changes on the way, namely variable naming and
> adding support for 8021AD proto.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>
> v3->v4:
> - don't try to be smart and copy skb->mac_len manipulation as it is. It can be
> adjusted later on.
I am fine with moving this code to generic library, as long as we can
adjust the mac_len as offset for MPLS header, otherwise this breaks
MPLS support.
> - fix error path in do_execute_actions as suggested by Pravin
> v2->v3:
> - used previously introduced __vlan_insert_tag helper
> - used skb_push/pull to get skb->data into needed point
> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
> v1->v2:
> - adjusted to fix recent ovs changes
> - included change to use make_writable suggested by Eric
>
> include/linux/skbuff.h | 2 +
> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> net/openvswitch/actions.c | 86 +++++-------------------------------------
> 3 files changed, 106 insertions(+), 77 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index e045516..78c299f 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
> int skb_ensure_writable(struct sk_buff *skb, int write_len);
> +int skb_vlan_pop(struct sk_buff *skb);
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>
> struct skb_checksum_ops {
> __wsum (*update)(const void *mem, int len, __wsum wsum);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d11bbe0..c906c5f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
> }
> EXPORT_SYMBOL(skb_ensure_writable);
>
> +/* remove VLAN header from packet and update csum accordingly. */
> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
> +{
> + struct vlan_hdr *vhdr;
> + unsigned int offset = skb->data - skb_mac_header(skb);
> + int err;
> +
> + __skb_push(skb, offset);
> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
> + if (unlikely(err))
> + goto pull;
> +
> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
> +
> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
> +
> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> + __skb_pull(skb, VLAN_HLEN);
> +
> + vlan_set_encap_proto(skb, vhdr);
> + skb->mac_header += VLAN_HLEN;
> +
> + if (skb_network_offset(skb) < ETH_HLEN)
> + skb_set_network_header(skb, ETH_HLEN);
> +
> + skb_reset_mac_len(skb);
> +pull:
> + __skb_pull(skb, offset);
> +
> + return err;
> +}
> +
> +int skb_vlan_pop(struct sk_buff *skb)
> +{
> + u16 vlan_tci;
> + __be16 vlan_proto;
> + int err;
> +
> + if (likely(vlan_tx_tag_present(skb))) {
> + skb->vlan_tci = 0;
> + } else {
> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
> + skb->protocol != htons(ETH_P_8021AD)) ||
> + skb->len < VLAN_ETH_HLEN))
> + return 0;
> +
> + err = __skb_vlan_pop(skb, &vlan_tci);
> + if (err)
> + return err;
> + }
> + /* move next vlan tag to hw accel tag */
> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
> + skb->protocol != htons(ETH_P_8021AD)) ||
> + skb->len < VLAN_ETH_HLEN))
> + return 0;
> +
> + vlan_proto = skb->protocol;
> + err = __skb_vlan_pop(skb, &vlan_tci);
> + if (unlikely(err))
> + return err;
> +
> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_pop);
> +
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
> +{
> + if (vlan_tx_tag_present(skb)) {
> + unsigned int offset = skb->data - skb_mac_header(skb);
> + int err;
> +
> + /* __vlan_insert_tag expect skb->data pointing to mac header.
> + * So change skb->data before calling it and change back to
> + * original position later
> + */
> + __skb_push(skb, offset);
> + err = __vlan_insert_tag(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (err)
> + return err;
> + skb->protocol = skb->vlan_proto;
> + skb->mac_len += VLAN_HLEN;
> + __skb_pull(skb, offset);
> +
> + if (skb->ip_summed == CHECKSUM_COMPLETE)
> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
> + }
> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_push);
> +
> /**
> * alloc_skb_with_frags - allocate skb with page frags
> *
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 7ffa377..4e05ea1 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> return 0;
> }
>
> -/* remove VLAN header from packet and update csum accordingly. */
> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
> -{
> - struct vlan_hdr *vhdr;
> - int err;
> -
> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
> - if (unlikely(err))
> - return err;
> -
> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
> -
> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> - *current_tci = vhdr->h_vlan_TCI;
> -
> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> - __skb_pull(skb, VLAN_HLEN);
> -
> - vlan_set_encap_proto(skb, vhdr);
> - skb->mac_header += VLAN_HLEN;
> -
> - if (skb_network_offset(skb) < ETH_HLEN)
> - skb_set_network_header(skb, ETH_HLEN);
> -
> - /* Update mac_len for subsequent MPLS actions */
> - skb_reset_mac_len(skb);
> - return 0;
> -}
> -
> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
> {
> - __be16 tci;
> int err;
>
> - if (likely(vlan_tx_tag_present(skb))) {
> - skb->vlan_tci = 0;
> - } else {
> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN))
> - return 0;
> -
> - err = __pop_vlan_tci(skb, &tci);
> - if (err)
> - return err;
> - }
> - /* move next vlan tag to hw accel tag */
> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN)) {
> + err = skb_vlan_pop(skb);
> + if (vlan_tx_tag_present(skb))
> + invalidate_flow_key(key);
> + else
> key->eth.tci = 0;
> - return 0;
> - }
> -
> - invalidate_flow_key(key);
> - err = __pop_vlan_tci(skb, &tci);
> - if (unlikely(err))
> - return err;
> -
> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
> - return 0;
> + return err;
> }
>
> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
> const struct ovs_action_push_vlan *vlan)
> {
> - if (unlikely(vlan_tx_tag_present(skb))) {
> - u16 current_tag;
> -
> - /* push down current VLAN tag */
> - current_tag = vlan_tx_tag_get(skb);
> -
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - current_tag);
> - if (!skb)
> - return -ENOMEM;
> - /* Update mac_len for subsequent MPLS actions */
> - skb->mac_len += VLAN_HLEN;
> -
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
> -
> + if (vlan_tx_tag_present(skb))
> invalidate_flow_key(key);
> - } else {
> + else
> key->eth.tci = vlan->vlan_tci;
> - }
> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
> - return 0;
> + return skb_vlan_push(skb, vlan->vlan_tpid,
> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
> }
>
> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
> @@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>
> case OVS_ACTION_ATTR_PUSH_VLAN:
> err = push_vlan(skb, key, nla_data(a));
> - if (unlikely(err)) /* skb already freed. */
> - return err;
> break;
>
> case OVS_ACTION_ATTR_POP_VLAN:
> --
> 1.9.3
>
^ permalink raw reply
* [PATCH v2 0/9] Replace smp_read_barrier_depends() with lockless_derefrence()
From: Pranith Kumar @ 2014-11-21 15:05 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Cristian Stoica, Horia Geanta,
Ruchika Gupta, Michael Neuling, Wolfram Sang,
open list:CRYPTO API, open list, Vinod Koul, Dan Williams,
Bartlomiej Zolnierkiewicz, Kyungmin Park, Manuel Schölling,
Dave Jiang, Rashika, open list:DMA GENERIC OFFLO...,
K. Y. Srinivasan, Haiyang Zhang
Recently lockless_dereference() was added which can be used in place of
hard-coding smp_read_barrier_depends().
http://lkml.iu.edu/hypermail/linux/kernel/1410.3/04561.html
The following series tries to do this.
There are still some hard-coded locations which I was not sure how to replace
with. I will send in separate patches/questions regarding them.
Pranith Kumar (9):
doc: memory-barriers.txt: Document use of lockless_dereference()
drivers: dma: Replace smp_read_barrier_depends() with
lockless_dereference()
dcache: Replace smp_read_barrier_depends() with lockless_dereference()
hyperv: Replace smp_read_barrier_depends() with lockless_dereference()
percpu: Replace smp_read_barrier_depends() with lockless_dereference()
perf: Replace smp_read_barrier_depends() with lockless_dereference()
seccomp: Replace smp_read_barrier_depends() with
lockless_dereference()
task_work: Replace smp_read_barrier_depends() with
lockless_dereference()
netfilter: Replace smp_read_barrier_depends() with
lockless_dereference()
Documentation/memory-barriers.txt | 4 ++--
drivers/dma/ioat/dma_v2.c | 3 +--
drivers/dma/ioat/dma_v3.c | 3 +--
fs/dcache.c | 7 ++-----
include/linux/hyperv.h | 9 ++++-----
include/linux/percpu-refcount.h | 4 +---
kernel/events/core.c | 3 +--
kernel/events/uprobes.c | 8 ++++----
kernel/seccomp.c | 7 +++----
kernel/task_work.c | 3 +--
net/ipv4/netfilter/arp_tables.c | 3 +--
net/ipv4/netfilter/ip_tables.c | 3 +--
net/ipv6/netfilter/ip6_tables.c | 3 +--
13 files changed, 23 insertions(+), 37 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v2 9/9] netfilter: Replace smp_read_barrier_depends() with lockless_dereference()
From: Pranith Kumar @ 2014-11-21 15:06 UTC (permalink / raw)
To: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, netfilter-devel, coreteam,
open list:NETWORKING [IPv4/..., open list
In-Reply-To: <1416582363-20661-1-git-send-email-bobby.prani@gmail.com>
Recently lockless_dereference() was added which can be used in place of
hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
net/ipv4/netfilter/arp_tables.c | 3 +--
net/ipv4/netfilter/ip_tables.c | 3 +--
net/ipv6/netfilter/ip6_tables.c | 3 +--
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index f95b6f9..fc7533d 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -270,12 +270,11 @@ unsigned int arpt_do_table(struct sk_buff *skb,
local_bh_disable();
addend = xt_write_recseq_begin();
- private = table->private;
/*
* Ensure we load private-> members after we've fetched the base
* pointer.
*/
- smp_read_barrier_depends();
+ private = lockless_dereference(table->private);
table_base = private->entries[smp_processor_id()];
e = get_entry(table_base, private->hook_entry[hook]);
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 99e810f..e0fd044 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -325,13 +325,12 @@ ipt_do_table(struct sk_buff *skb,
IP_NF_ASSERT(table->valid_hooks & (1 << hook));
local_bh_disable();
addend = xt_write_recseq_begin();
- private = table->private;
cpu = smp_processor_id();
/*
* Ensure we load private-> members after we've fetched the base
* pointer.
*/
- smp_read_barrier_depends();
+ private = lockless_dereference(table->private);
table_base = private->entries[cpu];
jumpstack = (struct ipt_entry **)private->jumpstack[cpu];
stackptr = per_cpu_ptr(private->stackptr, cpu);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index e080fbb..0459d6a 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -348,12 +348,11 @@ ip6t_do_table(struct sk_buff *skb,
local_bh_disable();
addend = xt_write_recseq_begin();
- private = table->private;
/*
* Ensure we load private-> members after we've fetched the base
* pointer.
*/
- smp_read_barrier_depends();
+ private = lockless_dereference(table->private);
cpu = smp_processor_id();
table_base = private->entries[cpu];
jumpstack = (struct ip6t_entry **)private->jumpstack[cpu];
--
1.9.1
^ permalink raw reply related
* Re: [RFC] situation with csum_and_copy_... API
From: Eric Dumazet @ 2014-11-21 15:01 UTC (permalink / raw)
To: Al Viro
Cc: David Miller, torvalds, netdev, linux-kernel, target-devel,
Nicholas A. Bellinger, Christoph Hellwig
In-Reply-To: <20141121084956.GT7996@ZenIV.linux.org.uk>
On Fri, 2014-11-21 at 08:49 +0000, Al Viro wrote:
> Another thing is tcp_sendmsg_fastopen() and tcp_send_rcvq(). The latter
> should just use copy_from_iter() instead of memcpy_from_iovec(), the former
> is dealt with by making tcp_send_syn_data() use the same copy_from_iter()
> instead of memcpy_from_iovecend().
Well, another problem I already mentioned is that tcp_send_rcvq() does a
single alloc_skb() with @size directly coming from user space. This
certainly can try allocation of dozen of Megabytes.
Not good.
^ permalink raw reply
* RE: [net-next v2 08/15] ixgbevf: compare total_rx_packets and budget in ixgbevf_clean_rx_irq
From: Tantilov, Emil S @ 2014-11-21 14:59 UTC (permalink / raw)
To: David Laight, Kirsher, Jeffrey T, davem@davemloft.net
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com, Alexander Duyck
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1C9F648C@AcuExch.aculab.com>
>-----Original Message-----
>From: David Laight [mailto:David.Laight@ACULAB.COM]
>Sent: Friday, November 21, 2014 3:08 AM
>To: Kirsher, Jeffrey T; davem@davemloft.net
>Cc: Tantilov, Emil S; netdev@vger.kernel.org;
>nhorman@redhat.com; sassmann@redhat.com;
>jogreene@redhat.com; Alexander Duyck
>Subject: RE: [net-next v2 08/15] ixgbevf: compare
>total_rx_packets and budget in ixgbevf_clean_rx_irq
>
>From: Jeff Kirsher
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>
>> total_rx_packets is the number of packets we had cleaned, and budget is
>> the total number of packets that we could clean per poll. Instead of
>> altering both of these values we can save ourselves one write to memory by
>> just comparing total_rx_packets to the budget and as long as we are less
>> than budget we continue cleaning.
>>
>> Also change the do{}while logic to while{} in order to avoid processing
>> packets when budget is 0.
>
>Does it matter if one packet is processed when budget is zero?
>The 'do {} while ();' version will generate better code.
Yes. The change is in sync with other drivers to protect against netpoll:
https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/patch/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c?id=57ba34c9b068f314b219affafc19a39f8735d5e8
Thanks,
Emil
>
> David
>
>
^ permalink raw reply
* [PATCH 4/4] defxx: Clean up DEFEA resource management
From: Maciej W. Rozycki @ 2014-11-21 14:10 UTC (permalink / raw)
To: netdev
In-Reply-To: <alpine.LFD.2.11.1411211107150.4773@eddie.linux-mips.org>
Reserve DEFEA resources according to actual use. There are three
regions, for the ESIC ASIC's CSRs, for the discrete Burst Holdoff
register, and for the PDQ ASIC's CSRs. The latter is mapped in the
memory or port I/O address space depending on configuration. The two
formers are hardwired and always mapped in the port I/O address space.
Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
linux-defea-resource.patch
Index: linux-20141025-dolch/drivers/net/fddi/defxx.c
===================================================================
--- linux-20141025-dolch.orig/drivers/net/fddi/defxx.c
+++ linux-20141025-dolch/drivers/net/fddi/defxx.c
@@ -414,7 +414,7 @@ static void dfx_port_read_long(DFX_board
* ================
*
* Overview:
- * Retrieves the address range used to access control and status
+ * Retrieves the address ranges used to access control and status
* registers.
*
* Returns:
@@ -422,8 +422,8 @@ static void dfx_port_read_long(DFX_board
*
* Arguments:
* bdev - pointer to device information
- * bar_start - pointer to store the start address
- * bar_len - pointer to store the length of the area
+ * bar_start - pointer to store the start addresses
+ * bar_len - pointer to store the lengths of the areas
*
* Assumptions:
* I am sure there are some.
@@ -442,8 +442,10 @@ static void dfx_get_bars(struct device *
if (dfx_bus_pci) {
int num = dfx_use_mmio ? 0 : 1;
- *bar_start = pci_resource_start(to_pci_dev(bdev), num);
- *bar_len = pci_resource_len(to_pci_dev(bdev), num);
+ bar_start[0] = pci_resource_start(to_pci_dev(bdev), num);
+ bar_len[0] = pci_resource_len(to_pci_dev(bdev), num);
+ bar_start[2] = bar_start[1] = 0;
+ bar_len[2] = bar_len[1] = 0;
}
if (dfx_bus_eisa) {
unsigned long base_addr = to_eisa_device(bdev)->base_addr;
@@ -457,24 +459,30 @@ static void dfx_get_bars(struct device *
bar_lo <<= 8;
bar_lo |= inb(base_addr + PI_ESIC_K_MEM_ADD_LO_CMP_0);
bar_lo <<= 8;
- *bar_start = bar_lo;
+ bar_start[0] = bar_lo;
bar_hi = inb(base_addr + PI_ESIC_K_MEM_ADD_HI_CMP_2);
bar_hi <<= 8;
bar_hi |= inb(base_addr + PI_ESIC_K_MEM_ADD_HI_CMP_1);
bar_hi <<= 8;
bar_hi |= inb(base_addr + PI_ESIC_K_MEM_ADD_HI_CMP_0);
bar_hi <<= 8;
- *bar_len = ((bar_hi - bar_lo) | PI_MEM_ADD_MASK_M) + 1;
+ bar_len[0] = ((bar_hi - bar_lo) | PI_MEM_ADD_MASK_M) +
+ 1;
} else {
- *bar_start = base_addr;
- *bar_len = PI_ESIC_K_CSR_IO_LEN +
- PI_ESIC_K_BURST_HOLDOFF_LEN;
+ bar_start[0] = base_addr;
+ bar_len[0] = PI_ESIC_K_CSR_IO_LEN;
}
+ bar_start[1] = base_addr + PI_DEFEA_K_BURST_HOLDOFF;
+ bar_len[1] = PI_ESIC_K_BURST_HOLDOFF_LEN;
+ bar_start[2] = base_addr + PI_ESIC_K_ESIC_CSR;
+ bar_len[2] = PI_ESIC_K_ESIC_CSR_LEN;
}
if (dfx_bus_tc) {
- *bar_start = to_tc_dev(bdev)->resource.start +
- PI_TC_K_CSR_OFFSET;
- *bar_len = PI_TC_K_CSR_LEN;
+ bar_start[0] = to_tc_dev(bdev)->resource.start +
+ PI_TC_K_CSR_OFFSET;
+ bar_len[0] = PI_TC_K_CSR_LEN;
+ bar_start[2] = bar_start[1] = 0;
+ bar_len[2] = bar_len[1] = 0;
}
}
@@ -525,8 +533,8 @@ static int dfx_register(struct device *b
const char *print_name = dev_name(bdev);
struct net_device *dev;
DFX_board_t *bp; /* board pointer */
- resource_size_t bar_start = 0; /* pointer to port */
- resource_size_t bar_len = 0; /* resource length */
+ resource_size_t bar_start[3]; /* pointers to ports */
+ resource_size_t bar_len[3]; /* resource length */
int alloc_size; /* total buffer size used */
struct resource *region;
int err = 0;
@@ -559,8 +567,8 @@ static int dfx_register(struct device *b
bp->bus_dev = bdev;
dev_set_drvdata(bdev, dev);
- dfx_get_bars(bdev, &bar_start, &bar_len);
- if (dfx_bus_eisa && dfx_use_mmio && bar_start == 0) {
+ dfx_get_bars(bdev, bar_start, bar_len);
+ if (dfx_bus_eisa && dfx_use_mmio && bar_start[0] == 0) {
pr_err("%s: Cannot use MMIO, no address set, aborting\n",
print_name);
pr_err("%s: Run ECU and set adapter's MMIO location\n",
@@ -572,28 +580,49 @@ static int dfx_register(struct device *b
}
if (dfx_use_mmio)
- region = request_mem_region(bar_start, bar_len, print_name);
+ region = request_mem_region(bar_start[0], bar_len[0],
+ print_name);
else
- region = request_region(bar_start, bar_len, print_name);
+ region = request_region(bar_start[0], bar_len[0], print_name);
if (!region) {
- printk(KERN_ERR "%s: Cannot reserve I/O resource "
- "0x%lx @ 0x%lx, aborting\n",
- print_name, (long)bar_len, (long)bar_start);
+ pr_err("%s: Cannot reserve %s resource 0x%lx @ 0x%lx, "
+ "aborting\n", dfx_use_mmio ? "MMIO" : "I/O", print_name,
+ (long)bar_len[0], (long)bar_start[0]);
err = -EBUSY;
goto err_out_disable;
}
+ if (bar_start[1] != 0) {
+ region = request_region(bar_start[1], bar_len[1], print_name);
+ if (!region) {
+ pr_err("%s: Cannot reserve I/O resource "
+ "0x%lx @ 0x%lx, aborting\n", print_name,
+ (long)bar_len[1], (long)bar_start[1]);
+ err = -EBUSY;
+ goto err_out_csr_region;
+ }
+ }
+ if (bar_start[2] != 0) {
+ region = request_region(bar_start[2], bar_len[2], print_name);
+ if (!region) {
+ pr_err("%s: Cannot reserve I/O resource "
+ "0x%lx @ 0x%lx, aborting\n", print_name,
+ (long)bar_len[2], (long)bar_start[2]);
+ err = -EBUSY;
+ goto err_out_bh_region;
+ }
+ }
/* Set up I/O base address. */
if (dfx_use_mmio) {
- bp->base.mem = ioremap_nocache(bar_start, bar_len);
+ bp->base.mem = ioremap_nocache(bar_start[0], bar_len[0]);
if (!bp->base.mem) {
printk(KERN_ERR "%s: Cannot map MMIO\n", print_name);
err = -ENOMEM;
- goto err_out_region;
+ goto err_out_esic_region;
}
} else {
- bp->base.port = bar_start;
- dev->base_addr = bar_start;
+ bp->base.port = bar_start[0];
+ dev->base_addr = bar_start[0];
}
/* Initialize new device structure */
@@ -602,7 +631,7 @@ static int dfx_register(struct device *b
if (dfx_bus_pci)
pci_set_master(to_pci_dev(bdev));
- if (dfx_driver_init(dev, print_name, bar_start) != DFX_K_SUCCESS) {
+ if (dfx_driver_init(dev, print_name, bar_start[0]) != DFX_K_SUCCESS) {
err = -ENODEV;
goto err_out_unmap;
}
@@ -630,11 +659,19 @@ static int dfx_register(struct device *b
if (dfx_use_mmio)
iounmap(bp->base.mem);
-err_out_region:
+err_out_esic_region:
+ if (bar_start[2] != 0)
+ release_region(bar_start[2], bar_len[2]);
+
+err_out_bh_region:
+ if (bar_start[1] != 0)
+ release_region(bar_start[1], bar_len[1]);
+
+err_out_csr_region:
if (dfx_use_mmio)
- release_mem_region(bar_start, bar_len);
+ release_mem_region(bar_start[0], bar_len[0]);
else
- release_region(bar_start, bar_len);
+ release_region(bar_start[0], bar_len[0]);
err_out_disable:
if (dfx_bus_pci)
@@ -1085,8 +1122,8 @@ static int dfx_driver_init(struct net_de
board_name = "DEFEA";
if (dfx_bus_pci)
board_name = "DEFPA";
- pr_info("%s: %s at %saddr = 0x%llx, IRQ = %d, Hardware addr = %pMF\n",
- print_name, board_name, dfx_use_mmio ? "" : "I/O ",
+ pr_info("%s: %s at %s addr = 0x%llx, IRQ = %d, Hardware addr = %pMF\n",
+ print_name, board_name, dfx_use_mmio ? "MMIO" : "I/O",
(long long)bar_start, dev->irq, dev->dev_addr);
/*
@@ -3660,8 +3697,8 @@ static void dfx_unregister(struct device
int dfx_bus_pci = dev_is_pci(bdev);
int dfx_bus_tc = DFX_BUS_TC(bdev);
int dfx_use_mmio = DFX_MMIO || dfx_bus_tc;
- resource_size_t bar_start = 0; /* pointer to port */
- resource_size_t bar_len = 0; /* resource length */
+ resource_size_t bar_start[3]; /* pointers to ports */
+ resource_size_t bar_len[3]; /* resource lengths */
int alloc_size; /* total buffer size used */
unregister_netdev(dev);
@@ -3679,12 +3716,16 @@ static void dfx_unregister(struct device
dfx_bus_uninit(dev);
- dfx_get_bars(bdev, &bar_start, &bar_len);
+ dfx_get_bars(bdev, bar_start, bar_len);
+ if (bar_start[2] != 0)
+ release_region(bar_start[2], bar_len[2]);
+ if (bar_start[1] != 0)
+ release_region(bar_start[1], bar_len[1]);
if (dfx_use_mmio) {
iounmap(bp->base.mem);
- release_mem_region(bar_start, bar_len);
+ release_mem_region(bar_start[0], bar_len[0]);
} else
- release_region(bar_start, bar_len);
+ release_region(bar_start[0], bar_len[0]);
if (dfx_bus_pci)
pci_disable_device(to_pci_dev(bdev));
Index: linux-20141025-dolch/drivers/net/fddi/defxx.h
===================================================================
--- linux-20141025-dolch.orig/drivers/net/fddi/defxx.h
+++ linux-20141025-dolch/drivers/net/fddi/defxx.h
@@ -1481,9 +1481,11 @@ typedef union
#define PI_ESIC_K_CSR_IO_LEN 0x40 /* 64 bytes */
#define PI_ESIC_K_BURST_HOLDOFF_LEN 0x04 /* 4 bytes */
+#define PI_ESIC_K_ESIC_CSR_LEN 0x40 /* 64 bytes */
#define PI_DEFEA_K_CSR_IO 0x000
#define PI_DEFEA_K_BURST_HOLDOFF 0x040
+#define PI_ESIC_K_ESIC_CSR 0xC80
#define PI_ESIC_K_SLOT_ID 0xC80
#define PI_ESIC_K_SLOT_CNTRL 0xC84
^ permalink raw reply
* [PATCH 3/4] defxx: Disable DEFEA's ESIC I/O decoding on shutdown
From: Maciej W. Rozycki @ 2014-11-21 14:10 UTC (permalink / raw)
To: netdev
In-Reply-To: <alpine.LFD.2.11.1411211107150.4773@eddie.linux-mips.org>
Make sure the option card does not respond after shutdown by disabling
it via ESIC's Expansion Board Control register. Also disable memory and
port I/O decoders, the latter in particular to disable slot-specific I/O
decoding that otherwise remains active even in the board is disabled.
Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
linux-defea-bus-uninit.patch
Index: linux-20141025-dolch/drivers/net/fddi/defxx.c
===================================================================
--- linux-20141025-dolch.orig/drivers/net/fddi/defxx.c
+++ linux-20141025-dolch/drivers/net/fddi/defxx.c
@@ -856,6 +856,12 @@ static void dfx_bus_uninit(struct net_de
val = inb(base_addr + PI_ESIC_K_IO_CONFIG_STAT_0);
val &= ~PI_CONFIG_STAT_0_M_INT_ENB;
outb(val, base_addr + PI_ESIC_K_IO_CONFIG_STAT_0);
+
+ /* Disable the board. */
+ outb(0, base_addr + PI_ESIC_K_SLOT_CNTRL);
+
+ /* Disable memory and port decoders. */
+ outb(0, base_addr + PI_ESIC_K_FUNCTION_CNTRL);
}
if (dfx_bus_pci) {
/* Disable interrupts at PCI bus interface chip (PFI) */
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox