Re: [PATCH] vti6: Add pmtu handling to vti6_xmit.

[Mark McKinstry]

I've tested this patch in our scenario and I can confirm that it still 
fixes all of our issues.

On 22/03/16 23:53, Steffen Klassert wrote:
> On Tue, Mar 15, 2016 at 01:28:01PM +0100, Steffen Klassert wrote:
>> On Mon, Mar 14, 2016 at 09:52:05PM +, Mark McKinstry wrote:
>>> Your patch adds a dst_release() call to my suggested fix, but this is
>>> problematic because the kfree_skb() call at tx_error already takes care
>>> of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() >
>>> skb_release_head_state() > skb_dst_drop()
>>>   > refdst_drop() > dst_release(). In our scenario your patch results in
>>> a negative refcount kernel warning being generated in dst_release() for
>>> every packet that is too big to go over the vti.
>> Hm. I've just noticed that my pmtu test does not trigger this
>> codepath, so I did not see the warning.
>>
>> Seems like we do the pmtu handling too late, it should happen before
>> we do skb_dst_set(). Also skb_scrub_packet() resets skb->ignore_df,
>> so checking ignore_df after skb_scrub_packet() does not make much sense.
>>
>> I'll send an updated version after some more testing.
>>
> I've added a testcase that triggers this codepath to my testing
> environment. The patch below works for me, could you please test
> if it fixes your problems?
>
> Subject: [PATCH] vti: Add pmtu handling to vti_xmit.
>
> We currently rely on the PMTU discovery of xfrm.
> However if a packet is locally sent, the PMTU mechanism
> of xfrm tries to do local socket notification what
> might not work for applications like ping that don't
> check for this. So add pmtu handling to vti_xmit to
> report MTU changes immediately.
>
> Signed-off-by: Steffen Klassert 
> ---
>   net/ipv4/ip_vti.c | 18 ++
>   1 file changed, 18 insertions(+)
>
> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index 5cf10b7..a917903 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
> @@ -156,6 +156,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct 
> net_device *dev,
>   struct dst_entry *dst = skb_dst(skb);
>   struct net_device *tdev;/* Device to other host */
>   int err;
> + int mtu;
>   
>   if (!dst) {
>   dev->stats.tx_carrier_errors++;
> @@ -192,6 +193,23 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct 
> net_device *dev,
>   tunnel->err_count = 0;
>   }
>   
> + mtu = dst_mtu(dst);
> + if (skb->len > mtu) {
> + skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
> + if (skb->protocol == htons(ETH_P_IP)) {
> + icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
> +   htonl(mtu));
> + } else {
> + if (mtu < IPV6_MIN_MTU)
> + mtu = IPV6_MIN_MTU;
> +
> + icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
> + }
> +
> + dst_release(dst);
> + goto tx_error;
> + }
> +
>   skb_scrub_packet(skb, !net_eq(tunnel->net, dev_net(dev)));
>   skb_dst_set(skb, dst);
>   skb->dev = skb_dst(skb)->dev;
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] crypto: AF_ALG - add support for key_id

[David Woodhouse]

> Tadeusz Struk  wrote:
>
>> +keyring = request_key(_type_asymmetric, key_name, NULL);
>> +
>> +err = -ENOKEY;
>> +if (IS_ERR(keyring))
>> +goto out;
>> +
>> +pkey = keyring->payload.data[asym_crypto];
>
> NAK.  This is liable to crash in future.  You may not assume that you know
> what keyring->payload.data[asym_crypto] points to.
>
> You may not use struct public_key outside of crypto/asymmetric_key/.  It's
> the
> internal data of the software subtype.  I'll move it out of the global
> header
> to remove the temptation;-).
>
> You must use accessor functions such as verify_signature().  Feel free to
> add
> further accessor functions such as query_asym_capabilities(),
> create_signature(), encrypt_blob() and decrypt_blob() or something like
> that.

Grr. This is not the first time this has been pointed out. And it
shouldn't have *needed* to be pointed out in the first place.

Please do not ignore review feedback.

Or common sense.


-- 
dwmw2

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] crypto: AF_ALG - add support for key_id

[David Woodhouse]

> Tadeusz Struk  wrote:
>
>> +keyring = request_key(_type_asymmetric, key_name, NULL);
>> +
>> +err = -ENOKEY;
>> +if (IS_ERR(keyring))
>> +goto out;
>> +
>> +pkey = keyring->payload.data[asym_crypto];
>
> NAK.  This is liable to crash in future.  You may not assume that you know
> what keyring->payload.data[asym_crypto] points to.
>
> You may not use struct public_key outside of crypto/asymmetric_key/.  It's
> the
> internal data of the software subtype.  I'll move it out of the global
> header
> to remove the temptation;-).
>
> You must use accessor functions such as verify_signature().  Feel free to
> add
> further accessor functions such as query_asym_capabilities(),
> create_signature(), encrypt_blob() and decrypt_blob() or something like
> that.

Grr. This is not the first time this has been pointed out. And it
shouldn't have *needed* to be pointed out in the first place.

Please do not ignore review feedback.

Or common sense.


-- 
dwmw2

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] crypto: AF_ALG - add support for key_id

[Tadeusz Struk]

Hi David,
On 03/30/2016 09:31 AM, David Howells wrote:
>> +keyring = request_key(_type_asymmetric, key_name, NULL);
>> > +
>> > +  err = -ENOKEY;
>> > +  if (IS_ERR(keyring))
>> > +  goto out;
>> > +
>> > +  pkey = keyring->payload.data[asym_crypto];
> NAK.  This is liable to crash in future.  You may not assume that you know
> what keyring->payload.data[asym_crypto] points to.
> 
> You may not use struct public_key outside of crypto/asymmetric_key/.  It's the
> internal data of the software subtype.  I'll move it out of the global header
> to remove the temptation;-).
> 
> You must use accessor functions such as verify_signature().  Feel free to add
> further accessor functions such as query_asym_capabilities(),
> create_signature(), encrypt_blob() and decrypt_blob() or something like that.

Thanks for your response. I thought that the public_key_query_sw_key(pkey)
check was enough for now.
I'll remove public_key stuff from af_alg and add the accessors.
Thanks,
-- 
TS
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] crypto: AF_ALG - add support for key_id

[David Howells]

Tadeusz Struk  wrote:

> + keyring = request_key(_type_asymmetric, key_name, NULL);
> +
> + err = -ENOKEY;
> + if (IS_ERR(keyring))
> + goto out;
> +
> + pkey = keyring->payload.data[asym_crypto];

NAK.  This is liable to crash in future.  You may not assume that you know
what keyring->payload.data[asym_crypto] points to.

You may not use struct public_key outside of crypto/asymmetric_key/.  It's the
internal data of the software subtype.  I'll move it out of the global header
to remove the temptation;-).

You must use accessor functions such as verify_signature().  Feel free to add
further accessor functions such as query_asym_capabilities(),
create_signature(), encrypt_blob() and decrypt_blob() or something like that.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Crypto Fixes for 4.6

[Herbert Xu]

Hi Linus:

This push fixes a bug in pkcs7_validate_trust and its users where
the output value may in fact be taken from uninitialised memory.


Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Nicolai Stange (1):
  PKCS#7: pkcs7_validate_trust(): initialize the _trusted output argument

 crypto/asymmetric_keys/pkcs7_trust.c |2 ++
 1 file changed, 2 insertions(+)

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html