On Mon, 3 Aug 2020 23:36:09 +0300
Vitaliy Makkoveev <[email protected]> wrote:
> On Tue, Aug 04, 2020 at 01:26:14AM +0900, YASUOKA Masahiko wrote:
>> Comments?
>
> You introduce `cookie' as
>
> cookie = session->protocol << 16 | session->session_id;
>
> also multicast sessions initialized as
>
> session->protocol = PIPEX_PROTO_NONE;
> session->session_id = ifindex;
>
> `protocol' and `session_id' come from userland, so I like to have checks
> like below. It's allow us to avoid `cookie' be broken while
> `pr_session_id' exceeds 16 bit integer. Also userland should not pass
> PIPEX_PROTO_NONE as `pr_protocol' because we shouldn't have multicast
> and not multicast sessions with the same `cookie'.
>
> ---- cut begin ----
>
> pipex_init_session(struct pipex_session **rsession,
> struct pipex_session_req *req)
> {
> if (req->pr_protocol == PIPEX_PROTO_NONE)
> return (EINVAL);
pipex_init_session() has the same check already.
287 int
288 pipex_init_session(struct pipex_session **rsession,
289 struct pipex_session_req *req)
290 {
(snip)
297 switch (req->pr_protocol) {
298 #ifdef PIPEX_PPPOE
299 case PIPEX_PROTO_PPPOE:
(snip)
333 default:
334 return (EPROTONOSUPPORT);
335 }
>
> if (req->pr_session_id > 0xffff)
> return (EINVAL);
>
> ---- cut end ----
req->pr_session_id can't be > 0xffff since it's uint16_t.
> Also cookies introduce invalidation problem. Yes, it has low
> probability, but we can have operation order like below:
>
> 1. enqueue session with `protocol' = 0xaa and `session_id' = 0xbb, and
> `cookie' = 0xaabb
> 2. kill this session
> 3. create new session `protocol' = 0xaa and `session_id' = 0xbb
> 4. this newly created session will be used by pipexintr()
>
> As I have seen while played with refcounters, session can be enqueued
> more than 10 times...
The diff makes the problem worse, but it could happen already if the
session-id is reused.
> Also It's not obvious that interface index will never exceed 16 bit
> counter. It's unsigned int and may be underlay counter's resolution
> will be expanded in future. So I like to have at least corresponding
> assertion in pipex_iface_init().
Right. This is fixable with another unique number.
> So, may be my first solution is the best here. And, as mpi@ pointed,
> ipsec(4) should be reworked to allow parallelism.
Does first mean killing the pipexintr?
What I explained was wrong. I'm sorry about this.
On Fri, 31 Jul 2020 09:36:32 +0900 (JST)
YASUOKA Masahiko <[email protected]> wrote:
> A packet of L2TP/IPsec (encapsulated IP/PPP/L2TP/UDP/ESP/UDP/IP) is
> processed like:
>
> ipv4_input
> ...
> udp_input
> ipsec_common_input
> esp_input
> crypto_dispatch
> => crypto_taskq_mp_safe
>
> kthread "crynlk"
> crypto_invoke
> ... (*1)
> crypto_done
> esp_input_cb
> ipsec_common_input_cb
> ip_deliver
> udp_input
> pipex_l2tp_input
> pipex_common_input
> (*2)
> pipex_ppp_input
> pipex_mppe_input (*3)
> pipex_ppp_input
> pipex_ip_input
> ipv4_input
> ...
This should be
kthread "crynlk"
crypto_invoke
... (*1)
crypto_done
kthread "crypto" <---- another thread
ipsec_input_cb <---- this is missed
esp_input_cb
ipsec_common_input_cb
ip_deliver
udp_input
pipex_l2tp_input
pipex_common_input
(*2)
pipex_ppp_input
pipex_mppe_input (*3)
pipex_ppp_input
pipex_ip_input
ipv4_input
...
> At *2 there was a queue. "crynlk" is a busy thread, since it is doing
> decryption at *1. I think it's better pipex input is be done by
> another thread than crypto since it also has decryption at *3.
This is false. *3 is done by another thread.
It is the same if crypto driver is not CRYPTOCAP_F_MPSAFE.
(crypto_invoke() is done by the caller's thread and the callback
(ipsec_input_cb) is called by"crypto" thread.)
So I have no actual reason to keep the queues.
ok yasuoka for the diff which kills pipexintr.
