On Tue, Aug 04, 2020 at 01:53:55PM +0900, YASUOKA Masahiko wrote:
> On Mon, 3 Aug 2020 23:36:09 +0300
> Vitaliy Makkoveev <m...@openbsd.org> 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.
My fault. Sorry.
>
> 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?
Yes.
>
> What I explained was wrong. I'm sorry about this.
>
> On Fri, 31 Jul 2020 09:36:32 +0900 (JST)
> YASUOKA Masahiko <yasu...@openbsd.org> 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.
>
Thanks for explanation and reviews.
