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
>
>
