Re: pipex(4): kill pipexintr()
On Fri, Jul 31, 2020 at 10:25:48PM +0200, Martin Pieuchot wrote: > On 31/07/20(Fri) 21:58, Vitaliy Makkoveev wrote: > > [...] > > What denies us to move pipex(4) under it's own lock? > > Such question won't lead us anywhere. It assumes it makes sense to move > pipex under its own lock. This assumption has many drawback which clearly > haven't been studied and more importantly it doesn't explains what for. > > What is your goal? What are you trying to achieve? Improve latency? > Improve performances? Of which subsystem? Where is the bottleneck? > What is the architecture of the system? > If I understood Yasuoka correctly kthread "crynlk" is the bottleneck and he wish MPPE be offloaded to another cpu. Since there is no simultaneous execution of crypto thread and pipexintr() which do MPPE stuff, the easiest way to reach it is to move pipex(4) under another lock. I don't mean "stop everything and implement now", I mean we can do it in the future. If there is no bottleneck in crypto thread, I see no reason to not remove pipexintr(). If the bottleneck is the crypto thread, I propose to implement refcounters to fix issue. And finish this thread. > IMHO the KERNEL_LOCK() should be removed an anything else postponed at > least until one has a clear understanding of the whole subsystem under > the NET_LOCK(). > The whole subsystem is under NET_LOCK() now. Just after we fix use after free issue we can start to move pipex(4) out of KERNEL_LOCK().
Re: pipex(4): kill pipexintr()
On 31/07/20(Fri) 21:58, Vitaliy Makkoveev wrote: > [...] > What denies us to move pipex(4) under it's own lock? Such question won't lead us anywhere. It assumes it makes sense to move pipex under its own lock. This assumption has many drawback which clearly haven't been studied and more importantly it doesn't explains what for. What is your goal? What are you trying to achieve? Improve latency? Improve performances? Of which subsystem? Where is the bottleneck? What is the architecture of the system? IMHO the KERNEL_LOCK() should be removed an anything else postponed at least until one has a clear understanding of the whole subsystem under the NET_LOCK().
Re: pipex(4): kill pipexintr()
Well, since pipexintr() killing was rejected, I propose to implement
reference counters to protect pipex(4) session itself. Diff below does
this.
Index: sys/net/if_ethersubr.c
===
RCS file: /cvs/src/sys/net/if_ethersubr.c,v
retrieving revision 1.266
diff -u -p -r1.266 if_ethersubr.c
--- sys/net/if_ethersubr.c 22 Jul 2020 02:16:01 - 1.266
+++ sys/net/if_ethersubr.c 31 Jul 2020 13:56:31 -
@@ -527,6 +527,7 @@ ether_input(struct ifnet *ifp, struct mb
if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
pipex_pppoe_input(m, session);
+ pipex_rele_session(session);
return;
}
}
Index: sys/net/if_gre.c
===
RCS file: /cvs/src/sys/net/if_gre.c,v
retrieving revision 1.158
diff -u -p -r1.158 if_gre.c
--- sys/net/if_gre.c10 Jul 2020 13:26:41 - 1.158
+++ sys/net/if_gre.c31 Jul 2020 13:56:31 -
@@ -984,9 +984,15 @@ gre_input_1(struct gre_tunnel *key, stru
struct pipex_session *session;
session = pipex_pptp_lookup_session(m);
- if (session != NULL &&
- pipex_pptp_input(m, session) == NULL)
- return (NULL);
+ if (session != NULL) {
+ struct mbuf *m0;
+
+ m0 = pipex_pptp_input(m, session);
+ pipex_rele_session(session);
+
+ if (m0 == NULL)
+ return NULL;
+ }
}
#endif
break;
Index: sys/net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.122
diff -u -p -r1.122 pipex.c
--- sys/net/pipex.c 29 Jul 2020 12:09:31 - 1.122
+++ sys/net/pipex.c 31 Jul 2020 13:56:32 -
@@ -165,6 +165,7 @@ pipex_iface_init(struct pipex_iface_cont
/* virtual pipex_session entry for multicast */
session = pool_get(&pipex_session_pool, PR_WAITOK | PR_ZERO);
+ session->refs = 1;
session->is_multicast = 1;
session->pipex_iface = pipex_iface;
session->ifindex = ifindex;
@@ -197,9 +198,9 @@ pipex_iface_stop(struct pipex_iface_cont
void
pipex_iface_fini(struct pipex_iface_context *pipex_iface)
{
- pool_put(&pipex_session_pool, pipex_iface->multicast_session);
NET_LOCK();
pipex_iface_stop(pipex_iface);
+ pipex_rele_session(pipex_iface->multicast_session);
NET_UNLOCK();
}
@@ -329,6 +330,7 @@ pipex_init_session(struct pipex_session
/* prepare a new session */
session = pool_get(&pipex_session_pool, PR_WAITOK | PR_ZERO);
+ session->refs = 1;
session->state = PIPEX_STATE_INITIAL;
session->protocol = req->pr_protocol;
session->session_id = req->pr_session_id;
@@ -421,9 +423,20 @@ pipex_init_session(struct pipex_session
return 0;
}
+void pipex_ref_session(struct pipex_session *session)
+{
+ atomic_inc_int_nv(&session->refs);
+ KASSERT(session->refs != 0);
+}
+
void
pipex_rele_session(struct pipex_session *session)
{
+ KASSERT(session->refs > 0);
+
+ if (atomic_dec_int_nv(&session->refs) > 0)
+ return;
+
if (session->mppe_recv.old_session_keys)
pool_put(&mppe_key_pool, session->mppe_recv.old_session_keys);
pool_put(&pipex_session_pool, session);
@@ -439,10 +452,12 @@ pipex_link_session(struct pipex_session
if (!iface->pipexmode)
return (ENXIO);
- if (pipex_lookup_by_session_id(session->protocol,
+ if (pipex_lookup_by_session_id_nonref(session->protocol,
session->session_id))
return (EEXIST);
+ pipex_ref_session(session);
+
session->pipex_iface = iface;
session->ifindex = iface->ifindex;
@@ -490,6 +505,8 @@ pipex_unlink_session(struct pipex_sessio
/* if final session is destroyed, stop timer */
if (LIST_EMPTY(&pipex_session_list))
pipex_timer_stop();
+
+ pipex_rele_session(session);
}
Static int
@@ -505,8 +522,8 @@ pipex_add_session(struct pipex_session_r
/* commit the session */
if (!in_nullhost(session->ip_address.sin_addr)) {
- if (pipex_lookup_by_ip_address(session->ip_address.sin_addr)
- != NULL) {
+ if (pipex_lookup_by_ip_address_nonref(
+ session->ip_address.sin_addr) != NULL) {
error = EADDRINUSE;
goto free;
}
@@ -568,6 +585,7 @@ pipex_close_session(struct pipex_session
s
Re: pipex(4): kill pipexintr()
On Fri, Jul 31, 2020 at 08:26:22PM +0200, Martin Pieuchot wrote:
> On 31/07/20(Fri) 12:15, Vitaliy Makkoveev wrote:
> > On Fri, Jul 31, 2020 at 09:36:32AM +0900, YASUOKA Masahiko wrote:
> > > On Thu, 30 Jul 2020 22:43:10 +0300
> > > Vitaliy Makkoveev wrote:
> > > > On Thu, Jul 30, 2020 at 10:05:13PM +0900, YASUOKA Masahiko wrote:
> > > >> On Thu, 30 Jul 2020 15:34:09 +0300
> > > >> Vitaliy Makkoveev wrote:
> > > >> > On Thu, Jul 30, 2020 at 09:13:46PM +0900, YASUOKA Masahiko wrote:
> > > >> >> Hi,
> > > >> >>
> > > >> >> sys/net/if_ethersubr.c:
> > > >> >> 372 void
> > > >> >> 373 ether_input(struct ifnet *ifp, struct mbuf *m)
> > > >> >> (snip)
> > > >> >> 519 #if NPPPOE > 0 || defined(PIPEX)
> > > >> >> 520 case ETHERTYPE_PPPOEDISC:
> > > >> >> 521 case ETHERTYPE_PPPOE:
> > > >> >> 522 if (m->m_flags & (M_MCAST | M_BCAST))
> > > >> >> 523 goto dropanyway;
> > > >> >> 524 #ifdef PIPEX
> > > >> >> 525 if (pipex_enable) {
> > > >> >> 526 struct pipex_session *session;
> > > >> >> 527
> > > >> >> 528 if ((session =
> > > >> >> pipex_pppoe_lookup_session(m)) != NULL) {
> > > >> >> 529 pipex_pppoe_input(m, session);
> > > >> >> 530 return;
> > > >> >> 531 }
> > > >> >> 532 }
> > > >> >> 533 #endif
> > > >> >>
> > > >> >> previously a packet which branchces to #529 is enqueued.
> > > >> >>
> > > >> >> If the diff removes the queue, then the pipex input routine is
> > > >> >> executed by the NIC's interrupt handler.
> > > >> >>
> > > >> >> The queues had been made to avoid that kind of situations.
> > > >> >
> > > >> > It's not enqueued in pppoe case. According pipex_pppoe_input() code
> > > >> > we
> > > >> > call pipex_common_input() with `useq' argument set to '0', so we
> > > >> > don't
> > > >> > enqueue mbuf(9) but pass it to pipex_ppp_input() which will pass it
> > > >> > to
> > > >> > ipv{4,6}_input().
> > > >>
> > > >> You are right. Sorry, I forgot about this which I did that by myself.
> > > >>
> > > >
> > > > I'm interesting the reason why you did that.
> > > >
> > > >> >> Also I don't see a relation of the use-after-free problem and
> > > >> >> killing
> > > >> >> queues. Can't we fix the problem unless we kill the queues?
> > > >> >
> > > >> > Yes we can. Reference counters allow us to keep orphan sessions in
> > > >> > these
> > > >> > queues without use after free issue.
> > > >> >
> > > >> > I will wait your commentaries current enqueuing before to do
> > > >> > something.
> > > >>
> > > >> I have another concern.
> > > >>
> > > >> You might know, when L2TP/IPsec is used heavily, the crypto thread
> > > >> uses 100% of 1 CPU core. In that case, that thread becomes like
> > > >> below:
> > > >>
> > > >> crypto thread -> udp_userreq -> pipex_l2tp_input
> > > >>
> > > >> some clients are using MPPE(RC4 encryption) on CCP. It's not so
> > > >> light.
> > > >>
> > > >> How do we offload this for CPUs? I am thinking that "pipex" can have
> > > >> a dedicated thread. Do we have another scenario?
> > > >>
> > > >
> > > > I suppose you mean udp_input(). What is you call "crypto thread"? I did
> > > > a little backtrace but I didn't find this thread.
> > > >
> > > > ether_resolve
> > > > if_input_local
> > > > ipv4_input
> > > > ip_input_if
> > > > ip_ours
> > > > ip_deliver
> > > > udp_input (through pr_input)
> > > > pipex_l2tp_input
> > > >
> > > > ipi{,6}_mloopback
> > > > if_input_local
> > > > ipv4_input
> > > > ...
> > > > udp_input (through pr_input)
> > > > pipex_l2tp_input
> > > >
> > > > loinput
> > > > if_input_local
> > > > ipv4_input
> > > > ...
> > > > udp_input (through pr_input)
> > > > pipex_l2tp_input
> > > >
> > > > Also various pseudo drivers call ipv{4,6}_input() and underlay
> > > > udp_unput() too.
> > > >
> > > > Except nfs, we call udp_usrreq() through socket layer only. Do you mean
> > > > userland as "crypto thread"?
> > >
> > > Sorry, udp_usrreq() should be usr_input() and crypto thread meant a
> > > kthread for crypto_taskq_mp_safe, whose name is "crynlk" (see
> > > crypto_init()).
> > >
> > > 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)
>
Re: pipex(4): kill pipexintr()
On 31/07/20(Fri) 12:15, Vitaliy Makkoveev wrote:
> On Fri, Jul 31, 2020 at 09:36:32AM +0900, YASUOKA Masahiko wrote:
> > On Thu, 30 Jul 2020 22:43:10 +0300
> > Vitaliy Makkoveev wrote:
> > > On Thu, Jul 30, 2020 at 10:05:13PM +0900, YASUOKA Masahiko wrote:
> > >> On Thu, 30 Jul 2020 15:34:09 +0300
> > >> Vitaliy Makkoveev wrote:
> > >> > On Thu, Jul 30, 2020 at 09:13:46PM +0900, YASUOKA Masahiko wrote:
> > >> >> Hi,
> > >> >>
> > >> >> sys/net/if_ethersubr.c:
> > >> >> 372 void
> > >> >> 373 ether_input(struct ifnet *ifp, struct mbuf *m)
> > >> >> (snip)
> > >> >> 519 #if NPPPOE > 0 || defined(PIPEX)
> > >> >> 520 case ETHERTYPE_PPPOEDISC:
> > >> >> 521 case ETHERTYPE_PPPOE:
> > >> >> 522 if (m->m_flags & (M_MCAST | M_BCAST))
> > >> >> 523 goto dropanyway;
> > >> >> 524 #ifdef PIPEX
> > >> >> 525 if (pipex_enable) {
> > >> >> 526 struct pipex_session *session;
> > >> >> 527
> > >> >> 528 if ((session =
> > >> >> pipex_pppoe_lookup_session(m)) != NULL) {
> > >> >> 529 pipex_pppoe_input(m, session);
> > >> >> 530 return;
> > >> >> 531 }
> > >> >> 532 }
> > >> >> 533 #endif
> > >> >>
> > >> >> previously a packet which branchces to #529 is enqueued.
> > >> >>
> > >> >> If the diff removes the queue, then the pipex input routine is
> > >> >> executed by the NIC's interrupt handler.
> > >> >>
> > >> >> The queues had been made to avoid that kind of situations.
> > >> >
> > >> > It's not enqueued in pppoe case. According pipex_pppoe_input() code we
> > >> > call pipex_common_input() with `useq' argument set to '0', so we don't
> > >> > enqueue mbuf(9) but pass it to pipex_ppp_input() which will pass it to
> > >> > ipv{4,6}_input().
> > >>
> > >> You are right. Sorry, I forgot about this which I did that by myself.
> > >>
> > >
> > > I'm interesting the reason why you did that.
> > >
> > >> >> Also I don't see a relation of the use-after-free problem and killing
> > >> >> queues. Can't we fix the problem unless we kill the queues?
> > >> >
> > >> > Yes we can. Reference counters allow us to keep orphan sessions in
> > >> > these
> > >> > queues without use after free issue.
> > >> >
> > >> > I will wait your commentaries current enqueuing before to do something.
> > >>
> > >> I have another concern.
> > >>
> > >> You might know, when L2TP/IPsec is used heavily, the crypto thread
> > >> uses 100% of 1 CPU core. In that case, that thread becomes like
> > >> below:
> > >>
> > >> crypto thread -> udp_userreq -> pipex_l2tp_input
> > >>
> > >> some clients are using MPPE(RC4 encryption) on CCP. It's not so
> > >> light.
> > >>
> > >> How do we offload this for CPUs? I am thinking that "pipex" can have
> > >> a dedicated thread. Do we have another scenario?
> > >>
> > >
> > > I suppose you mean udp_input(). What is you call "crypto thread"? I did
> > > a little backtrace but I didn't find this thread.
> > >
> > > ether_resolve
> > > if_input_local
> > > ipv4_input
> > > ip_input_if
> > > ip_ours
> > > ip_deliver
> > > udp_input (through pr_input)
> > > pipex_l2tp_input
> > >
> > > ipi{,6}_mloopback
> > > if_input_local
> > > ipv4_input
> > > ...
> > > udp_input (through pr_input)
> > > pipex_l2tp_input
> > >
> > > loinput
> > > if_input_local
> > > ipv4_input
> > > ...
> > > udp_input (through pr_input)
> > > pipex_l2tp_input
> > >
> > > Also various pseudo drivers call ipv{4,6}_input() and underlay
> > > udp_unput() too.
> > >
> > > Except nfs, we call udp_usrreq() through socket layer only. Do you mean
> > > userland as "crypto thread"?
> >
> > Sorry, udp_usrreq() should be usr_input() and crypto thread meant a
> > kthread for crypto_taskq_mp_safe, whose name is "crynlk" (see
> > crypto_init()).
> >
> > 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
> > ...
> >
> > At *2 there was a queue. "crynlk" is a busy thread, since it is doing
> > decryption at *1. I think it's
xhci(4): fix for usbd_start_next: error=13
When playing around with uvideo(4) devices I'm quite regular hitting the error message in the subject when closing the device. The problem seems to be some false return code ordering in xhci_device_isoc_start(): if (sc->sc_bus.dying || xp->halted) return (USBD_IOERROR); /* Why would you do that anyway? */ if (sc->sc_bus.use_polling) return (USBD_INVAL); /* * To allow continuous transfers, above we start all transfers * immediately. However, we're still going to get usbd_start_next call * this when another xfer completes. So, check if this is already * in progress or not */ if (xx->ntrb > 0) return (USBD_IN_PROGRESS); When we close the device, xhci_abort_xfer() will set xp->halted. When usbd_start_next() tries to dequeue an transfer afterwards which is already in progress, xhci_device_isoc_start() will first check for xp->halted and return USBD_IOERROR to usbd_start_next() which finally results in the error message been print. By re-ordering the USBD_IN_PROGRESS check to be first, the same way as ehci(4) does, this gets fixed. Feedback? OK? Index: xhci.c === RCS file: /cvs/src/sys/dev/usb/xhci.c,v retrieving revision 1.118 diff -u -p -u -p -r1.118 xhci.c --- xhci.c 29 Jul 2020 16:37:12 - 1.118 +++ xhci.c 31 Jul 2020 16:45:52 - @@ -3111,13 +3111,6 @@ xhci_device_isoc_start(struct usbd_xfer KASSERT(!(xfer->rqflags & URQ_REQUEST)); - if (sc->sc_bus.dying || xp->halted) - return (USBD_IOERROR); - - /* Why would you do that anyway? */ - if (sc->sc_bus.use_polling) - return (USBD_INVAL); - /* * To allow continuous transfers, above we start all transfers * immediately. However, we're still going to get usbd_start_next call @@ -3126,6 +3119,13 @@ xhci_device_isoc_start(struct usbd_xfer */ if (xx->ntrb > 0) return (USBD_IN_PROGRESS); + + if (sc->sc_bus.dying || xp->halted) + return (USBD_IOERROR); + + /* Why would you do that anyway? */ + if (sc->sc_bus.use_polling) + return (USBD_INVAL); paddr = DMAADDR(&xfer->dmabuf, 0);
Re: usbd_abort_pipe(); usbd_close_pipe; dance
On 31/07/20(Fri) 11:22, Marcus Glocker wrote:
> Maybe I'm missing something here.
You aren't. Historically usbd_close_pipe() wasn't aborting transfers.
We changed it to do so as it happened to be the easiest fix to some
issues that had been copy/pasted.
It's just that nobody took the time to do the cleanup you're now
suggesting, thanks!
> But is there any specific reason why the most of our USB drivers are
> calling usbd_abort_pipe() right before usbd_close_pipe()? Since
> usbd_close_pipe() already will call usbd_abort_pipe() if the pipe isn't
> empty, as documented in the man page:
>
> DESCRIPTION
> The usbd_abort_pipe() function aborts any transfers queued on pipe.
>
> The usbd_close_pipe() function aborts any transfers queued on pipe
> then deletes it.
>
> In case this happened because of an inherited copy/paste chain, can we
> nuke the superfluous usbd_abort_pipe() calls?
Yes please, ok mpi@
> Index: if_atu.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
> retrieving revision 1.131
> diff -u -p -u -p -r1.131 if_atu.c
> --- if_atu.c 10 Jul 2020 13:26:40 - 1.131
> +++ if_atu.c 31 Jul 2020 08:26:24 -
> @@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
>
> /* Stop transfers. */
> if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
> - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> if (err) {
> DPRINTF(("%s: close rx pipe failed: %s\n",
> @@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
> }
>
> if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
> - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> if (err) {
> DPRINTF(("%s: close tx pipe failed: %s\n",
> Index: if_aue.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
> retrieving revision 1.110
> diff -u -p -u -p -r1.110 if_aue.c
> --- if_aue.c 10 Jul 2020 13:26:40 - 1.110
> +++ if_aue.c 31 Jul 2020 08:26:25 -
> @@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
>
> /* Stop transfers. */
> if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
> - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> if (err) {
> printf("%s: close rx pipe failed: %s\n",
> @@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
> }
>
> if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
> - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> if (err) {
> printf("%s: close tx pipe failed: %s\n",
> @@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
> }
>
> if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
> - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> if (err) {
> printf("%s: close intr pipe failed: %s\n",
> Index: if_axe.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
> retrieving revision 1.141
> diff -u -p -u -p -r1.141 if_axe.c
> --- if_axe.c 10 Jul 2020 13:26:40 - 1.141
> +++ if_axe.c 31 Jul 2020 08:26:25 -
> @@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
>
> /* Stop transfers. */
> if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
> - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> if (err) {
> printf("axe%d: close rx pipe failed: %s\n",
> @@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
> }
>
> if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
> - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> if (err) {
> printf("axe%d: close tx pipe failed: %s\n",
> @@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
> }
>
> if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
> - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> if (err) {
> printf("axe%d: close intr pipe failed: %s\n",
> Index: if_axen.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
> retrieving revision 1.29
> diff -u -p -u -p -r1.29 if_axen.c
> --- if_axen.c 10 Jul 2020 13:26:40 - 1.29
> +++ if_axen.c 31 Jul 2020 08:26:25 -
> @@ -1426,7 +1426,6 @@ axen_stop(struct axen_softc *sc)
>
> /* Stop transfers. */
> if (sc->axen_ep[AXEN_
Re: cat(1): add more restrictive pledge(2)
As a general rule, pledge uses isn't supposed to make programs
more complicated.
Stuart Henderson wrote:
> On 2020/07/31 00:07, tempmai...@firemail.cc wrote:
> > I have to say I'm only a beginner to C but hopefully my patch is
> > good.
> >
> > This patch adds a second and more restrictive pledge (only "stdio"
> > instead of "stdio rpath") after the getopt loop if there is no
> > input file or if the input file is "-" (stdin) or a sequence of
> > repeated instances of "-". It doesn't move argv past the last "-",
> > and doesn't pledge if it runs into an input file other than "-".
> >
> > I've compiled it and tested it with ktrace(1) and kdump(1) and it
> > appears to work as expected and pledge correctly with at least
> > these invocations:
> > $ echo test | ./cat -uv # pledge("stdio", NULL);
> > $ echo test | ./cat -uv - # pledge("stdio", NULL);
> > $ echo test | ./cat # pledge("stdio", NULL);
> > $ echo test | ./cat - - - # pledge("stdio", NULL);
> > $ echo test | ./cat - # pledge("stdio", NULL);
> > $ echo test | ./cat - cat.c # pledge("stdio rpath", NULL);
> > $ echo test | ./cat cat.c - # pledge("stdio rpath", NULL);
> >
> >
> >
> > Index: bin/cat/cat.c
> > ===
> > RCS file: /cvs/src/bin/cat/cat.c,v
> > retrieving revision 1.27
> > diff -u -p -u -p -r1.27 cat.c
> > --- bin/cat/cat.c 28 Jun 2019 13:34:58 - 1.27
> > +++ bin/cat/cat.c 30 Jul 2020 23:21:14 -
> > @@ -94,7 +94,26 @@ main(int argc, char *argv[])
> > "usage: %s [-benstuv] [file ...]\n", __progname);
> > return 1;
> > }
> > + argc -= optind;
> > argv += optind;
> > +
> > + if (argc) {
> > + if (!strcmp(*argv, "-")) {
> > + do {
> > + if (argc == 1) {
> > + if (pledge("stdio", NULL) == -1)
> > + err(1, "pledge");
> > + argc--, argv++;
> > + break;
> > + } else
> > + argc--, argv++;
> > + } while (argc && !strcmp(*argv, "-"));
> > + argc++, argv--;
> > + }
> > + } else {
> > + if (pledge("stdio", NULL) == -1)
> > + err(1, "pledge");
> > + }
> >
> > if (bflag || eflag || nflag || sflag || tflag || vflag)
> > cook_args(argv);
> >
>
> The improvement is fairly small; cat doesn't have network access or the
> ability to write files with the previous pledge. Is this worth the
> considerable extra complexity?
>
> It's hard to get a feel for whether the argc/argv manipulation is correct.
>
Re: usbd_abort_pipe(); usbd_close_pipe; dance
On Fri, 31 Jul 2020 11:59:45 +0200
Gerhard Roth wrote:
> Hi Marcus,
>
> On 2020-07-31 11:22, Marcus Glocker wrote:
> > Maybe I'm missing something here.
> >
> > But is there any specific reason why the most of our USB drivers are
> > calling usbd_abort_pipe() right before usbd_close_pipe()? Since
> > usbd_close_pipe() already will call usbd_abort_pipe() if the pipe
> > isn't empty, as documented in the man page:
> >
> > DESCRIPTION
> > The usbd_abort_pipe() function aborts any transfers queued on
> > pipe.
> >
> > The usbd_close_pipe() function aborts any transfers queued on
> > pipe then deletes it.
> >
> > In case this happened because of an inherited copy/paste chain, can
> > we nuke the superfluous usbd_abort_pipe() calls?
>
> I was asking myself the same question before ;)
>
> Apparently, the call to usbd_abort_pipe() inside of usbd_close_pipe()
> wasn't there right from the start. It was added by mpi@ with rev 1.57
> of usbdi.c abort 7 years ago. Hence drivers written before that
> needed the the usbd_abort_pipe(). But that is no longer the case.
Ah right, that explains it then - Thanks!
> ok gerhard@
>
>
>
>
> >
> >
> > Index: if_atu.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
> > retrieving revision 1.131
> > diff -u -p -u -p -r1.131 if_atu.c
> > --- if_atu.c10 Jul 2020 13:26:40 - 1.131
> > +++ if_atu.c31 Jul 2020 08:26:24 -
> > @@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
> >
> > /* Stop transfers. */
> > if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
> > - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> > err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> > if (err) {
> > DPRINTF(("%s: close rx pipe failed: %s\n",
> > @@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
> > }
> >
> > if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
> > - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> > err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> > if (err) {
> > DPRINTF(("%s: close tx pipe failed: %s\n",
> > Index: if_aue.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
> > retrieving revision 1.110
> > diff -u -p -u -p -r1.110 if_aue.c
> > --- if_aue.c10 Jul 2020 13:26:40 - 1.110
> > +++ if_aue.c31 Jul 2020 08:26:25 -
> > @@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
> >
> > /* Stop transfers. */
> > if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
> > - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> > err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> > if (err) {
> > printf("%s: close rx pipe failed: %s\n",
> > @@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
> > }
> >
> > if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
> > - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> > err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> > if (err) {
> > printf("%s: close tx pipe failed: %s\n",
> > @@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
> > }
> >
> > if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
> > - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> > err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> > if (err) {
> > printf("%s: close intr pipe failed: %s\n",
> > Index: if_axe.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
> > retrieving revision 1.141
> > diff -u -p -u -p -r1.141 if_axe.c
> > --- if_axe.c10 Jul 2020 13:26:40 - 1.141
> > +++ if_axe.c31 Jul 2020 08:26:25 -
> > @@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
> >
> > /* Stop transfers. */
> > if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
> > - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> > err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> > if (err) {
> > printf("axe%d: close rx pipe failed:
> > %s\n", @@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
> > }
> >
> > if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
> > - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> > err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> > if (err) {
> > printf("axe%d: close tx pipe failed:
> > %s\n", @@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
> > }
> >
> > if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
> > - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> > err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> > if (err) {
> > printf("axe%d: close intr pipe failed:
> > %s\n", Index: if_axen.c
> > ===
Re: usbd_abort_pipe(); usbd_close_pipe; dance
Hi Marcus,
On 2020-07-31 11:22, Marcus Glocker wrote:
Maybe I'm missing something here.
But is there any specific reason why the most of our USB drivers are
calling usbd_abort_pipe() right before usbd_close_pipe()? Since
usbd_close_pipe() already will call usbd_abort_pipe() if the pipe isn't
empty, as documented in the man page:
DESCRIPTION
The usbd_abort_pipe() function aborts any transfers queued on pipe.
The usbd_close_pipe() function aborts any transfers queued on pipe
then deletes it.
In case this happened because of an inherited copy/paste chain, can we
nuke the superfluous usbd_abort_pipe() calls?
I was asking myself the same question before ;)
Apparently, the call to usbd_abort_pipe() inside of usbd_close_pipe()
wasn't there right from the start. It was added by mpi@ with rev 1.57 of
usbdi.c abort 7 years ago. Hence drivers written before that needed the
the usbd_abort_pipe(). But that is no longer the case.
ok gerhard@
Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.131
diff -u -p -u -p -r1.131 if_atu.c
--- if_atu.c10 Jul 2020 13:26:40 - 1.131
+++ if_atu.c31 Jul 2020 08:26:24 -
@@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
/* Stop transfers. */
if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
if (err) {
DPRINTF(("%s: close rx pipe failed: %s\n",
@@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
}
if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
if (err) {
DPRINTF(("%s: close tx pipe failed: %s\n",
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.110
diff -u -p -u -p -r1.110 if_aue.c
--- if_aue.c10 Jul 2020 13:26:40 - 1.110
+++ if_aue.c31 Jul 2020 08:26:25 -
@@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
/* Stop transfers. */
if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
if (err) {
printf("%s: close rx pipe failed: %s\n",
@@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
if (err) {
printf("%s: close tx pipe failed: %s\n",
@@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
if (err) {
printf("%s: close intr pipe failed: %s\n",
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.141
diff -u -p -u -p -r1.141 if_axe.c
--- if_axe.c10 Jul 2020 13:26:40 - 1.141
+++ if_axe.c31 Jul 2020 08:26:25 -
@@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
/* Stop transfers. */
if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
if (err) {
printf("axe%d: close rx pipe failed: %s\n",
@@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
if (err) {
printf("axe%d: close tx pipe failed: %s\n",
@@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
if (err) {
printf("axe%d: close intr pipe failed: %s\n",
Index: if_axen.c
===
RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 if_axen.c
--- if_axen.c 10 Jul 2020 13:26:40 - 1.29
+++ if_axen.c 31 Jul 2020 08:26:25 -
@@ -1426,7 +1426,6 @@ axen_stop(struct axen_softc *sc)
/* Stop transfers. */
if (sc->axen_ep[AXEN_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->a
usbd_abort_pipe(); usbd_close_pipe; dance
Maybe I'm missing something here.
But is there any specific reason why the most of our USB drivers are
calling usbd_abort_pipe() right before usbd_close_pipe()? Since
usbd_close_pipe() already will call usbd_abort_pipe() if the pipe isn't
empty, as documented in the man page:
DESCRIPTION
The usbd_abort_pipe() function aborts any transfers queued on pipe.
The usbd_close_pipe() function aborts any transfers queued on pipe
then deletes it.
In case this happened because of an inherited copy/paste chain, can we
nuke the superfluous usbd_abort_pipe() calls?
Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.131
diff -u -p -u -p -r1.131 if_atu.c
--- if_atu.c10 Jul 2020 13:26:40 - 1.131
+++ if_atu.c31 Jul 2020 08:26:24 -
@@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
/* Stop transfers. */
if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
if (err) {
DPRINTF(("%s: close rx pipe failed: %s\n",
@@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
}
if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
if (err) {
DPRINTF(("%s: close tx pipe failed: %s\n",
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.110
diff -u -p -u -p -r1.110 if_aue.c
--- if_aue.c10 Jul 2020 13:26:40 - 1.110
+++ if_aue.c31 Jul 2020 08:26:25 -
@@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
/* Stop transfers. */
if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
if (err) {
printf("%s: close rx pipe failed: %s\n",
@@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
if (err) {
printf("%s: close tx pipe failed: %s\n",
@@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
if (err) {
printf("%s: close intr pipe failed: %s\n",
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.141
diff -u -p -u -p -r1.141 if_axe.c
--- if_axe.c10 Jul 2020 13:26:40 - 1.141
+++ if_axe.c31 Jul 2020 08:26:25 -
@@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
/* Stop transfers. */
if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
if (err) {
printf("axe%d: close rx pipe failed: %s\n",
@@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
if (err) {
printf("axe%d: close tx pipe failed: %s\n",
@@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
if (err) {
printf("axe%d: close intr pipe failed: %s\n",
Index: if_axen.c
===
RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 if_axen.c
--- if_axen.c 10 Jul 2020 13:26:40 - 1.29
+++ if_axen.c 31 Jul 2020 08:26:25 -
@@ -1426,7 +1426,6 @@ axen_stop(struct axen_softc *sc)
/* Stop transfers. */
if (sc->axen_ep[AXEN_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->axen_ep[AXEN_ENDPT_RX]);
err = usbd_close_pipe(sc->axen_ep[AXEN_ENDPT_RX]);
if (err) {
printf("axen%d: close rx pipe failed: %s\n",
@@ -1436,7 +1435,6 @@ axen_stop(struct axen_softc *sc)
}
if (sc->axen_ep[AXEN_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->axen_ep[AXEN_ENDPT_TX]);
Re: pipex(4): kill pipexintr()
On Fri, Jul 31, 2020 at 09:36:32AM +0900, YASUOKA Masahiko wrote:
> On Thu, 30 Jul 2020 22:43:10 +0300
> Vitaliy Makkoveev wrote:
> > On Thu, Jul 30, 2020 at 10:05:13PM +0900, YASUOKA Masahiko wrote:
> >> On Thu, 30 Jul 2020 15:34:09 +0300
> >> Vitaliy Makkoveev wrote:
> >> > On Thu, Jul 30, 2020 at 09:13:46PM +0900, YASUOKA Masahiko wrote:
> >> >> Hi,
> >> >>
> >> >> sys/net/if_ethersubr.c:
> >> >> 372 void
> >> >> 373 ether_input(struct ifnet *ifp, struct mbuf *m)
> >> >> (snip)
> >> >> 519 #if NPPPOE > 0 || defined(PIPEX)
> >> >> 520 case ETHERTYPE_PPPOEDISC:
> >> >> 521 case ETHERTYPE_PPPOE:
> >> >> 522 if (m->m_flags & (M_MCAST | M_BCAST))
> >> >> 523 goto dropanyway;
> >> >> 524 #ifdef PIPEX
> >> >> 525 if (pipex_enable) {
> >> >> 526 struct pipex_session *session;
> >> >> 527
> >> >> 528 if ((session =
> >> >> pipex_pppoe_lookup_session(m)) != NULL) {
> >> >> 529 pipex_pppoe_input(m, session);
> >> >> 530 return;
> >> >> 531 }
> >> >> 532 }
> >> >> 533 #endif
> >> >>
> >> >> previously a packet which branchces to #529 is enqueued.
> >> >>
> >> >> If the diff removes the queue, then the pipex input routine is
> >> >> executed by the NIC's interrupt handler.
> >> >>
> >> >> The queues had been made to avoid that kind of situations.
> >> >
> >> > It's not enqueued in pppoe case. According pipex_pppoe_input() code we
> >> > call pipex_common_input() with `useq' argument set to '0', so we don't
> >> > enqueue mbuf(9) but pass it to pipex_ppp_input() which will pass it to
> >> > ipv{4,6}_input().
> >>
> >> You are right. Sorry, I forgot about this which I did that by myself.
> >>
> >
> > I'm interesting the reason why you did that.
> >
> >> >> Also I don't see a relation of the use-after-free problem and killing
> >> >> queues. Can't we fix the problem unless we kill the queues?
> >> >
> >> > Yes we can. Reference counters allow us to keep orphan sessions in these
> >> > queues without use after free issue.
> >> >
> >> > I will wait your commentaries current enqueuing before to do something.
> >>
> >> I have another concern.
> >>
> >> You might know, when L2TP/IPsec is used heavily, the crypto thread
> >> uses 100% of 1 CPU core. In that case, that thread becomes like
> >> below:
> >>
> >> crypto thread -> udp_userreq -> pipex_l2tp_input
> >>
> >> some clients are using MPPE(RC4 encryption) on CCP. It's not so
> >> light.
> >>
> >> How do we offload this for CPUs? I am thinking that "pipex" can have
> >> a dedicated thread. Do we have another scenario?
> >>
> >
> > I suppose you mean udp_input(). What is you call "crypto thread"? I did
> > a little backtrace but I didn't find this thread.
> >
> > ether_resolve
> > if_input_local
> > ipv4_input
> > ip_input_if
> > ip_ours
> > ip_deliver
> > udp_input (through pr_input)
> > pipex_l2tp_input
> >
> > ipi{,6}_mloopback
> > if_input_local
> > ipv4_input
> > ...
> > udp_input (through pr_input)
> > pipex_l2tp_input
> >
> > loinput
> > if_input_local
> > ipv4_input
> > ...
> > udp_input (through pr_input)
> > pipex_l2tp_input
> >
> > Also various pseudo drivers call ipv{4,6}_input() and underlay
> > udp_unput() too.
> >
> > Except nfs, we call udp_usrreq() through socket layer only. Do you mean
> > userland as "crypto thread"?
>
> Sorry, udp_usrreq() should be usr_input() and crypto thread meant a
> kthread for crypto_taskq_mp_safe, whose name is "crynlk" (see
> crypto_init()).
>
> 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
>...
>
> 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.
Thanks for explanation. esp_input_cb() and underlay routines called
under NET_LOCK() so we have no simultaneous execution in this path too.
With current locking scheme pipexinrt() only introduces delay
Re: cat(1): add more restrictive pledge(2)
On 2020/07/31 00:07, tempmai...@firemail.cc wrote:
> I have to say I'm only a beginner to C but hopefully my patch is
> good.
>
> This patch adds a second and more restrictive pledge (only "stdio"
> instead of "stdio rpath") after the getopt loop if there is no
> input file or if the input file is "-" (stdin) or a sequence of
> repeated instances of "-". It doesn't move argv past the last "-",
> and doesn't pledge if it runs into an input file other than "-".
>
> I've compiled it and tested it with ktrace(1) and kdump(1) and it
> appears to work as expected and pledge correctly with at least
> these invocations:
> $ echo test | ./cat -uv # pledge("stdio", NULL);
> $ echo test | ./cat -uv - # pledge("stdio", NULL);
> $ echo test | ./cat # pledge("stdio", NULL);
> $ echo test | ./cat - - - # pledge("stdio", NULL);
> $ echo test | ./cat - # pledge("stdio", NULL);
> $ echo test | ./cat - cat.c # pledge("stdio rpath", NULL);
> $ echo test | ./cat cat.c - # pledge("stdio rpath", NULL);
>
>
>
> Index: bin/cat/cat.c
> ===
> RCS file: /cvs/src/bin/cat/cat.c,v
> retrieving revision 1.27
> diff -u -p -u -p -r1.27 cat.c
> --- bin/cat/cat.c 28 Jun 2019 13:34:58 - 1.27
> +++ bin/cat/cat.c 30 Jul 2020 23:21:14 -
> @@ -94,7 +94,26 @@ main(int argc, char *argv[])
> "usage: %s [-benstuv] [file ...]\n", __progname);
> return 1;
> }
> + argc -= optind;
> argv += optind;
> +
> + if (argc) {
> + if (!strcmp(*argv, "-")) {
> + do {
> + if (argc == 1) {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> + argc--, argv++;
> + break;
> + } else
> + argc--, argv++;
> + } while (argc && !strcmp(*argv, "-"));
> + argc++, argv--;
> + }
> + } else {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> + }
>
> if (bflag || eflag || nflag || sflag || tflag || vflag)
> cook_args(argv);
>
The improvement is fairly small; cat doesn't have network access or the
ability to write files with the previous pledge. Is this worth the
considerable extra complexity?
It's hard to get a feel for whether the argc/argv manipulation is correct.
ifconfig: print tpmr(4) members
This diff is to be applied on top of my other diff on tech@ with subject
"ifconfig: merge switch_status() into bridge_status()".
It hooks completes the output of tpmr intefaces in what I think is the
simplest and least intrusive way.
tpmr is a trivial bridge and has no specific ioctls, so to distinguish
it from the rest we must rely on the interface name; assuming that it
is tpmr because neither is_bridge() nor is_switch() return success is
not possible due to the way ifconfig is designed: it runs all *_status()
commands for all interface types.
An alternative approach would be to make ifconfig try all the various
bridge related ioctls on all bridge-like interfaces and quiet down all
failures such output stays clean, but I dislike this shotgun approach
and prefer testing for different drivers where possible.
With this last piece in, I could finally document tpmr under ifconfig(8)
(and move on the next drivers in need of love).
Feedback? OK?
--- brconfig.c.orig Fri Jul 31 08:58:03 2020
+++ brconfig.c Fri Jul 31 09:16:59 2020
@@ -775,15 +775,28 @@
return (1);
}
+/* no tpmr(4) specific ioctls, name is enough if ifconfig.c:printif() passed */
+int
+is_tpmr(void)
+{
+ return (strncmp(ifname, "tpmr", sizeof("tpmr") - 1) == 0);
+}
+
void
bridge_status(void)
{
struct ifbrparam bp1, bp2;
- int isswitch = is_switch();
+ int isswitch;
+ if (is_tpmr()) {
+ bridge_list("\t");
+ return;
+ }
+
if (!is_bridge())
return;
+ isswitch = is_switch();
if (isswitch)
switch_cfg("\t");
else
