Re: Porting umb(4) from OpenBSD

[Pierre Pronchery <khorben%defora.org@localhost>]

			Hey tech-net@, (Maxime)

On 03/05/2018 09:58, Maxime Villard wrote:
> Le 03/05/2018 à 04:46, Pierre Pronchery a écrit :
>> You can obtain the fix(es) and follow my changes as I go here:
>> https://git.edgebsd.org/gitweb/?p=src.git;a=shortlog;h=refs/heads/khorben/umb
> 
>     int         umb_match(device_t, cfdata_t, void *);
>     [etc]
> please make all of that static, global symbols are bad

Done.

>     uint8_t umb_qmi_alloc_cid[] = {
>     [etc]
> if that's not supposed to move it should be const (and static too), also
> take care of the functions that use that, they should use const too

Done.

>     ifp->if_mtu = 1500;        /* use a common default */
>     ifp->if_mtu = sc->sc_maxpktlen;
> 
> this doesn't look nice

True; this is still this way in the OpenBSD driver, but I can revisit it
later anyway. For the record:

> umb0: flags=8810<POINTOPOINT,SIMPLEX,MULTICAST> mtu 2048

(the problem with ifconfig(8) really was in umb_ioctl())

>     sc->sc_resp_buf = kmem_alloc(sc->sc_ctrl_len, M_NOWAIT);
> 
> that's the wrong flag

Mmh it is this way in the original driver. Is there a specific
requirement on our side there?

>     #if 0
>             if ((error = suser(p)) != 0)
>                 break;
>     #endif
> 
> kauth, no?

Thanks for the hint, I will revisit this one too.

>         error = copyout(&sc->sc_info, ifr->ifr_data,
>             sizeof (sc->sc_info));
> 
> maybe we should explicitly zero out sc_info in umb_attach, perhaps it
> already is though

AFAIK it already is, at least when attaching.

>     m_copydata(m, 0, len, ptr + 1);
> 
> can you add a KASSERT please, to make sure 'len' is smaller than the area
> on ptr+1

Sure, I added this one:

> KASSERT(len <= sc->sc_tx_bufsz - sizeof(*hdr) - sizeof(*ptr));

Which I believe is correct; second pair of eyes welcome.

>     all the sizeof (x)
> 
> please remove the whitespace, sizeof(x)

Done too.

For anyone willing to test the driver, I have also written very basic
code for a hypothetical umbctl(8). I figured we could have something
like that, akin to pppoectl(8), instead of adding weight to ifconfig(8).
The current code is attached here.

If it looks fine this way, I can work some more on it and change "-a
apn" to "apn=value" etc after the interface name instead, which is
closer to the way pppoectl(8) works.

And thanks for the review, it helped a lot already!

Cheers,
-- 
khorben

/* $NetBSD$ */
/*
 * Copyright (c) 2018 Pierre Pronchery <khorben%defora.org@localhost>
 *
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 *
 * THIS SOFTWARE IS PROVIDED BY THE DEVELOPERS ``AS IS'' AND ANY EXPRESS OR
 * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
 * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
 * IN NO EVENT SHALL THE DEVELOPERS BE LIABLE FOR ANY DIRECT, INDIRECT,
 * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
 * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
 * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
 * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */



#include <sys/ioctl.h>
#include <sys/socket.h>
#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <net/if.h>


struct umb_parameter
{
	int			op;
	int			is_puk;
#define MBIM_PIN_MAXLEN		32
	char			pin[MBIM_PIN_MAXLEN];
	int			pinlen;

	char			newpin[MBIM_PIN_MAXLEN];
	int			newpinlen;

#define UMB_APN_MAXLEN		100
	uint16_t		apn[UMB_APN_MAXLEN];
	int			apnlen;

	int			roaming;
	uint32_t		preferredclasses;
};

#ifndef SIOCSUMBPARAM
# define SIOCSUMBPARAM	_IOW('i', 191, struct ifreq)	/* set MBIM param */
#endif
#ifndef SIOCGUMBPARAM
# define SIOCGUMBPARAM	_IOWR('i', 192, struct ifreq)	/* get MBIM param */
#endif

static int _char_to_utf16(const char *in, uint16_t *out, size_t outlen);
static char const * _print_stars(size_t count);
static int _usage(void);

/* this function is from OpenBSD's ifconfig(8) */
static int _char_to_utf16(const char *in, uint16_t *out, size_t outlen)
{
	int	n = 0;
	uint16_t c;

	for (;;) {
		c = *in++;

		if (c == '\0') {
			/*
			 * NUL termination is not required, but zero out the
			 * residual buffer
			 */
			memset(out, 0, outlen);
			return n;
		}
		if (outlen < sizeof(*out))
			return -1;

		*out++ = htole16(c);
		n += sizeof(*out);
		outlen -= sizeof(*out);
	}
}

static char const * _print_stars(size_t count)
{
	static char buf[MBIM_PIN_MAXLEN + 1];
	size_t i;

	for(i = 0; i < count && i < sizeof(buf) - 1; i++)
		buf[i] = '*';
	buf[i] = '\0';
	return buf;
}

static int _usage(void)
{
	fputs("Usage: umbctl interface\n"
"       umbctl -a apn interface\n", stderr);
	return 1;
}

int main(int argc, char * argv[])
{
	int o;
	int fd;
	struct ifreq ifr;
	struct umb_parameter mp;
	char const * ifname;
	char const * apn = NULL;

	while((o = getopt(argc, argv, "a:")) != -1)
		switch(o)
		{
			case 'a':
				apn = optarg;
				break;
			default:
				return _usage();
		}
	if(optind + 1 != argc)
		return _usage();
	ifname = argv[optind];
	if((fd = socket(AF_INET, SOCK_DGRAM, 0)) < 0)
	{
		perror("socket");
		return 2;
	}
	memset(&ifr, 0, sizeof(ifr));
	strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
	memset(&mp, 0, sizeof(mp));
	ifr.ifr_data = &mp;
	if(ioctl(fd, SIOCGUMBPARAM, &ifr) != 0)
	{
		perror("SIOCGUMBPARAM");
		return 2;
	}
	if(apn == NULL)
		printf("%s: %s %s, roaming %d, classes %u\n", ifname,
				mp.is_puk ? "PUK" : "PIN",
				_print_stars(mp.pinlen), mp.roaming,
				mp.preferredclasses);
	else
	{
		if((mp.apnlen = _char_to_utf16(apn, mp.apn,
						sizeof(mp.apn))) < 0)
		{
			fprintf(stderr, "%s: APN too long\n", apn);
			return 3;
		}
		if(ioctl(fd, SIOCSUMBPARAM, &ifr) != 0)
		{
			perror("SIOCSUMBPARAM");
			return 4;
		}
	}
	return 0;
}