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.
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.
Would it make sense to have two separated diffs?
> ---- 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
