On Sat, Apr 04, 2020 at 06:52:49PM +0200, Martin Pieuchot wrote: > On 02/04/20(Thu) 13:44, Vitaliy Makkoveev wrote: > > This diff add ownership checks to the rest pipex_ioctl() commands. A few > > words about pppx_get_closed(): since in-kernel timeout feature was > > disabled for pppx(4) related pipex_sessions, closed pipex_sessions can't > > exist in system, so this function is dummy. I have an idea how to > > reenable this disabled timeout, but some reafactoring requited, and fair > > pipex_ioctl(PIPEXGCLOSED) call will be restored. > > The ownership looks good to me. > > I'd suggest we return an error for PIPEXGCLOSED documenting why this > ioctl(2) is no longer supported. That could have been part of the > previous fix that disabled the timeout. No need for a separate > function.
We will restore timeout later. And after some additional steps PIPEXGCLOSED will return fair list of PIPEX_STATE_CLOSE_WAIT stated sessions associated with pppx_dev. > See there's a conceptual problem with pipex_get_closed(). This function > never returns an error, so when npppd(8) will issue the ioctl, even if > an error occurs it will proceed with empty data. npppd(8) does this check. See usr.sbin/npppd/npppd/npppd.c:1327 > > Would it make sense to have two separated diffs? I split it because only PEXDSESSION will crash. But since these diff add the same checks they can be combined to one. > > > ---- cut begin > > diff --git sys/net/if_pppx.c sys/net/if_pppx.c > > index 37a6af0..6c4977d 100644 > > --- sys/net/if_pppx.c > > +++ sys/net/if_pppx.c > > @@ -175,6 +175,12 @@ int pppx_add_session(struct pppx_dev *, > > struct pipex_session_req *); > > int pppx_del_session(struct pppx_dev *, > > struct pipex_session_close_req *); > > +int pppx_config_session(struct pppx_dev *, > > + struct pipex_session_config_req *); > > +int pppx_get_stat(struct pppx_dev *, > > + struct pipex_session_stat_req *); > > +int pppx_get_closed(struct pppx_dev *, > > + struct pipex_session_list_req *); > > int pppx_set_session_descr(struct pppx_dev *, > > struct pipex_session_descr_req *); > > > > @@ -451,16 +457,18 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t addr, int > > flags, struct proc *p) > > break; > > > > case PIPEXCSESSION: > > - error = pipex_config_session( > > + error = pppx_config_session(pxd, > > (struct pipex_session_config_req *)addr); > > break; > > > > case PIPEXGSTAT: > > - error = pipex_get_stat((struct pipex_session_stat_req *)addr); > > + error = pppx_get_stat(pxd, > > + (struct pipex_session_stat_req *)addr); > > break; > > > > case PIPEXGCLOSED: > > - error = pipex_get_closed((struct pipex_session_list_req *)addr); > > + error = pppx_get_closed(pxd, > > + (struct pipex_session_list_req *)addr); > > break; > > > > case PIPEXSIFDESCR: > > @@ -947,6 +955,40 @@ pppx_del_session(struct pppx_dev *pxd, struct > > pipex_session_close_req *req) > > return (0); > > } > > > > +int > > +pppx_config_session(struct pppx_dev *pxd, > > + struct pipex_session_config_req *req) > > +{ > > + struct pppx_if *pxi; > > + > > + pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol); > > + if (pxi == NULL) > > + return (EINVAL); > > + > > + return pipex_config_session(req, &pxi->pxi_ifcontext); > > +} > > + > > +int > > +pppx_get_stat(struct pppx_dev *pxd, struct pipex_session_stat_req *req) > > +{ > > + struct pppx_if *pxi; > > + > > + pxi = pppx_if_find(pxd, req->psr_session_id, req->psr_protocol); > > + if (pxi == NULL) > > + return (EINVAL); > > + > > + return pipex_get_stat(req, &pxi->pxi_ifcontext); > > +} > > + > > +int > > +pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req) > > +{ > > + /* XXX: Only opened sessions exist for pppx(4) */ > > + memset(req, 0, sizeof(*req)); > > + > > + return 0; > > +} > > + > > int > > pppx_set_session_descr(struct pppx_dev *pxd, > > struct pipex_session_descr_req *req) > > diff --git sys/net/pipex.c sys/net/pipex.c > > index 22edce3..219e18d 100644 > > --- sys/net/pipex.c > > +++ sys/net/pipex.c > > @@ -235,15 +235,17 @@ pipex_ioctl(struct pipex_iface_context *pipex_iface, > > u_long cmd, caddr_t data) > > > > case PIPEXCSESSION: > > ret = pipex_config_session( > > - (struct pipex_session_config_req *)data); > > + (struct pipex_session_config_req *)data, pipex_iface); > > break; > > > > case PIPEXGSTAT: > > - ret = pipex_get_stat((struct pipex_session_stat_req *)data); > > + ret = pipex_get_stat((struct pipex_session_stat_req *)data, > > + pipex_iface); > > break; > > > > case PIPEXGCLOSED: > > - ret = pipex_get_closed((struct pipex_session_list_req *)data); > > + ret = pipex_get_closed((struct pipex_session_list_req *)data, > > + pipex_iface); > > break; > > > > default: > > @@ -514,7 +516,8 @@ pipex_close_session(struct pipex_session_close_req *req, > > } > > > > Static int > > -pipex_config_session(struct pipex_session_config_req *req) > > +pipex_config_session(struct pipex_session_config_req *req, > > + struct pipex_iface_context *iface) > > { > > struct pipex_session *session; > > > > @@ -523,36 +526,44 @@ pipex_config_session(struct pipex_session_config_req > > *req) > > req->pcr_session_id); > > if (session == NULL) > > return (EINVAL); > > + if (session->pipex_iface != iface) > > + return (EINVAL); > > session->ip_forward = req->pcr_ip_forward; > > > > return (0); > > } > > > > Static int > > -pipex_get_stat(struct pipex_session_stat_req *req) > > +pipex_get_stat(struct pipex_session_stat_req *req, > > + struct pipex_iface_context *iface) > > { > > struct pipex_session *session; > > > > NET_ASSERT_LOCKED(); > > session = pipex_lookup_by_session_id(req->psr_protocol, > > req->psr_session_id); > > - if (session == NULL) { > > + if (session == NULL) > > + return (EINVAL); > > + if (session->pipex_iface != iface) > > return (EINVAL); > > - } > > req->psr_stat = session->stat; > > > > return (0); > > } > > > > Static int > > -pipex_get_closed(struct pipex_session_list_req *req) > > +pipex_get_closed(struct pipex_session_list_req *req, > > + struct pipex_iface_context *iface) > > { > > - struct pipex_session *session; > > + struct pipex_session *session, *session_next; > > > > NET_ASSERT_LOCKED(); > > bzero(req, sizeof(*req)); > > - while (!LIST_EMPTY(&pipex_close_wait_list)) { > > - session = LIST_FIRST(&pipex_close_wait_list); > > + for (session = LIST_FIRST(&pipex_close_wait_list); > > + session; session = session_next) { > > + session_next = LIST_NEXT(session, state_list); > > + if (session->pipex_iface != iface) > > + continue; > > req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id; > > LIST_REMOVE(session, state_list); > > session->state = PIPEX_STATE_CLOSE_WAIT2; > > diff --git sys/net/pipex_local.h sys/net/pipex_local.h > > index ad3c3d3..c124eb9 100644 > > --- sys/net/pipex_local.h > > +++ sys/net/pipex_local.h > > @@ -371,9 +371,12 @@ void pipex_iface_stop (struct > > pipex_iface_context *); > > int pipex_add_session (struct pipex_session_req *, > > struct pipex_iface_context *); > > int pipex_close_session (struct pipex_session_close_req > > *, > > struct pipex_iface_context *); > > -int pipex_config_session (struct > > pipex_session_config_req *); > > -int pipex_get_stat (struct pipex_session_stat_req *); > > -int pipex_get_closed (struct pipex_session_list_req *); > > +int pipex_config_session (struct > > pipex_session_config_req *, > > + struct pipex_iface_context *); > > +int pipex_get_stat (struct pipex_session_stat_req *, > > + struct pipex_iface_context *); > > +int pipex_get_closed (struct pipex_session_list_req *, > > + struct pipex_iface_context *); > > int pipex_destroy_session (struct pipex_session *); > > struct pipex_session *pipex_lookup_by_ip_address (struct in_addr); > > struct pipex_session *pipex_lookup_by_session_id (int, int); > > ---- cut end > >