|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [UNIKRAFT PATCH v2 05/15] plat/tap: Get/Set hw_addr
Hi,
update on my comments/questions after a discussion off-list with
Sharan.
thanks!
Hugo
On Wed, 2020-07-08 at 14:04 +0200, Hugo Lefeuvre wrote:
> Hi Sharan,
>
> Looks good to me so far. I only have a comment/question concerning
> tap_mac_generate (inline).
>
> regards,
> Hugo
>
> On Tue, 2020-06-30 at 11:58 +0200, Sharan Santhanam wrote:
> >
> > The patch implements support for generating a default hw_addr. The
> > patch also implements get and set netdev api.
> >
> > Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> > ---
> > plat/drivers/include/tap/tap.h | 2 +
> > plat/drivers/tap/tap.c | 97
> > ++++++++++++++++++++++++++++-
> > plat/linuxu/include/linuxu/syscall-arm_32.h | 1 +
> > plat/linuxu/include/linuxu/syscall-x86_64.h | 1 +
> > plat/linuxu/include/linuxu/syscall.h | 25 ++++++++
> > plat/linuxu/include/linuxu/tap.h | 7 +++
> > plat/linuxu/tap_io.c | 31 +++++++++
> > 7 files changed, 161 insertions(+), 3 deletions(-)
> >
> > diff --git a/plat/drivers/include/tap/tap.h
> > b/plat/drivers/include/tap/tap.h
> > index 3aa90b6..686b666 100644
> > --- a/plat/drivers/include/tap/tap.h
> > +++ b/plat/drivers/include/tap/tap.h
> > @@ -45,5 +45,7 @@
> > int tap_open(__u32 flags);
> > int tap_close(int fd);
> > int tap_dev_configure(int fd, __u32 feature_flags, void *arg);
> > +int tap_netif_configure(int fd, __u32 request, void *arg);
> > +int tap_netif_create(void);
> >
> > #endif /* __PLAT_DRV_TAP_H */
> > diff --git a/plat/drivers/tap/tap.c b/plat/drivers/tap/tap.c
> > index acf958a..d60ca46 100644
> > --- a/plat/drivers/tap/tap.c
> > +++ b/plat/drivers/tap/tap.c
> > @@ -92,12 +92,16 @@ struct tap_net_dev {
> > UK_TAILQ_HEAD(tap_txqs, struct uk_netdev_tx_queue) txqs;
> > /* The list of the tap device */
> > UK_TAILQ_ENTRY(struct tap_net_dev) next;
> > + /* Mac address of the device */
> > + struct uk_hwaddr hw_addr;
> > /* Tap Device identifier */
> > __u16 tid;
> > /* UK Netdevice identifier */
> > __u16 id;
> > /* File Descriptor for the tap device */
> > int tap_fd;
> > + /* Control socket descriptor */
> > + int ctrl_sock;
> > /* Name of the character device */
> > char name[IFNAMSIZ];
> > /* MTU of the device */
> > @@ -170,6 +174,7 @@ static int tap_netdev_rxq_info_get(struct
> > uk_netdev *dev, __u16 queue_id,
> > static int tap_netdev_txq_info_get(struct uk_netdev *dev, __u16
> > queue_id,
> > struct uk_netdev_queue_info
> > *qinfo);
> > static int tap_device_create(struct tap_net_dev *tdev, __u32
> > feature_flags);
> > +static int tap_mac_generate(__u8 *addr, int len, __u8 dev_id);
> >
> > /**
> > * Local function definitions
> > @@ -284,16 +289,78 @@ static int tap_netdev_mtu_set(struct
> > uk_netdev
> > *n, __u16 mtu __unused)
> >
> > static const struct uk_hwaddr *tap_netdev_mac_get(struct uk_netdev
> > *n)
> > {
> > + struct tap_net_dev *tdev;
> > +
> > UK_ASSERT(n);
> > - return NULL;
> > + tdev = to_tapnetdev(n);
> > + return &tdev->hw_addr;
> > }
> >
> > static int tap_netdev_mac_set(struct uk_netdev *n,
> > const struct uk_hwaddr *hwaddr)
> > {
> > - int rc = -EINVAL;
> > + int rc = 0;
> > + struct tap_net_dev *tdev;
> > + struct uk_ifreq ifrq = {0};
> >
> > UK_ASSERT(n && hwaddr);
> > + tdev = to_tapnetdev(n);
> > +
> > + snprintf(ifrq.ifr_name, sizeof(ifrq.ifr_name), "%s", tdev-
> > >
> > > name);
> > + uk_pr_info("Setting mac address on tap device %s\n", tdev-
> > >
> > > name);
> > +
> > +#ifdef CONFIG_TAP_DEV_DEBUG
> > + int i;
> > +
> > + for (i = 0; i < UK_NETDEV_HWADDR_LEN; i++) {
> > + uk_pr_debug("hw_address: %d - %d\n", i,
> > + hwaddr->addr_bytes[i] & 0xFF);
> > + }
> > +#endif /* CONFIG_TAP_DEV_DEBUG */
> > +
> > + ifrq.ifr_hwaddr.sa_family = AF_LOCAL;
> > + memcpy(&ifrq.ifr_hwaddr.sa_data[0], &hwaddr-
> > >addr_bytes[0],
> > + UK_NETDEV_HWADDR_LEN);
> > + rc = tap_netif_configure(tdev->ctrl_sock,
> > UK_SIOCSIFHWADDR,
> > &ifrq);
> > + if (rc < 0) {
> > + uk_pr_err(DRIVER_NAME": Failed(%d) to set the
> > hardware address\n",
> > + rc);
> > + goto exit;
> > + }
> > + memcpy(&tdev->hw_addr, hwaddr, sizeof(*hwaddr));
> > +
> > +exit:
> > + return rc;
> > +}
> > +
> > +static int tap_mac_generate(__u8 *addr, int len, __u8 dev_id)
> > +{
> > + const char fmt[] = {0x2, 0x0, 0x0, 0x0, 0x0, 0x0};
> > +
> > + UK_ASSERT(addr && len > 0);
> > +
> > + if (len < UK_NETDEV_HWADDR_LEN) {
> > + uk_pr_err("Failed to generate the mac address\n");
> > + return -ENOSPC;
> > + }
> What is the point of this `len` argument?
>
> What happens if `len` is larger than UK_NETDEV_HWADDR_LEN?
>
The idea of this `len` argument was to pass the size of the addr
buffer. tap_mac_generate can then verify that it is big enough to hold
the mac address.
However, this is an internal function so we can assume that the caller
passes a sufficiently large buffer. And in any case, there's no point
in providing tap_mac_generate with a buffer that is not big enough to
hold a mac address...
Let's just remove this argument altogether?
> To me this might result in OOB read if the caller assumes that `addr`
> contains `len` > UK_NETDEV_HWADDR_LEN bytes of address data upon
> returning of this function.
My bad, this is no OOB read, more a read of uninitialized data.
>
> >
> > + memcpy(addr, fmt, UK_NETDEV_HWADDR_LEN - 1);
> > + *(addr + UK_NETDEV_HWADDR_LEN - 1) = (__u8) (dev_id + 1);
> > + return 0;
> > +}
> > +
> > +static inline int tapdev_ctrlsock_create(struct tap_net_dev *tdev)
> > +{
> > + int rc = 0;
> > +
> > + rc = tap_netif_create();
> > + if (rc < 0) {
> > + uk_pr_err(DRIVER_NAME":Failed(%d) to create a
> > control socket\n",
> > + rc);
> > + goto exit;
> > + }
> > + tdev->ctrl_sock = rc;
> > + rc = 0;
> > +exit:
> > return rc;
> > }
> >
> > @@ -328,13 +395,37 @@ static int tap_netdev_configure(struct
> > uk_netdev *n,
> > goto exit;
> > }
> >
> > - /* Initialize tx/rx queues list */
> > + /* Create a control socket for the network interface */
> > + rc = tapdev_ctrlsock_create(tdev);
> > + if (rc != 0) {
> > + uk_pr_err(DRIVER_NAME": Failed to create a control
> > socket\n");
> > + goto close_tap_dev;
> > + }
> > +
> > + /* Generate MAC address */
> > + tap_mac_generate(&tdev->hw_addr.addr_bytes[0],
> > UK_NETDEV_HWADDR_LEN,
> > + tdev->id);
> > +
> > + /* MAC Address configuration */
> > + rc = tap_netdev_mac_set(n, &tdev->hw_addr);
> > + if (rc < 0) {
> > + uk_pr_err(DRIVER_NAME": Failed to set the mac
> > address\n");
> > + goto close_ctrl_sock;
> > + }
> > +
> > + /* Initialize the tx/rx queues */
> > UK_TAILQ_INIT(&tdev->rxqs);
> > tdev->rxq_cnt = 0;
> > UK_TAILQ_INIT(&tdev->txqs);
> > tdev->txq_cnt = 0;
> > exit:
> > return rc;
> > +
> > +close_ctrl_sock:
> > + tap_close(tdev->ctrl_sock);
> > +close_tap_dev:
> > + tap_close(tdev->tap_fd);
> > + goto exit;
> > }
> >
> > static int tap_device_create(struct tap_net_dev *tdev, __u32
> > feature_flags)
> > diff --git a/plat/linuxu/include/linuxu/syscall-arm_32.h
> > b/plat/linuxu/include/linuxu/syscall-arm_32.h
> > index 093fd62..4b7218b 100644
> > --- a/plat/linuxu/include/linuxu/syscall-arm_32.h
> > +++ b/plat/linuxu/include/linuxu/syscall-arm_32.h
> > @@ -56,6 +56,7 @@
> > #define __SC_TIMER_GETOVERRUN 260
> > #define __SC_TIMER_DELETE 261
> > #define __SC_CLOCK_GETTIME 263
> > +#define __SC_SOCKET 281
> > #define __SC_PSELECT6 335
> >
> > #ifndef O_TMPFILE
> > diff --git a/plat/linuxu/include/linuxu/syscall-x86_64.h
> > b/plat/linuxu/include/linuxu/syscall-x86_64.h
> > index c4b88fc..8714c92 100644
> > --- a/plat/linuxu/include/linuxu/syscall-x86_64.h
> > +++ b/plat/linuxu/include/linuxu/syscall-x86_64.h
> > @@ -47,6 +47,7 @@
> > #define __SC_RT_SIGACTION 13
> > #define __SC_RT_SIGPROCMASK 14
> > #define __SC_IOCTL 16
> > +#define __SC_SOCKET 41
> > #define __SC_EXIT 60
> > #define __SC_FCNTL 72
> > #define __SC_ARCH_PRCTL 158
> > diff --git a/plat/linuxu/include/linuxu/syscall.h
> > b/plat/linuxu/include/linuxu/syscall.h
> > index 2c613fc..ad6f038 100644
> > --- a/plat/linuxu/include/linuxu/syscall.h
> > +++ b/plat/linuxu/include/linuxu/syscall.h
> > @@ -125,6 +125,31 @@ static inline int sys_close(int fd)
> > (long) fd);
> > }
> >
> > +#ifndef SOCK_STREAM
> > +#define SOCK_STREAM 1
> > +#endif /* SOCK_STREAM */
> > +#ifndef SOCK_DGRAM
> > +#define SOCK_DGRAM 2
> > +#endif /* SOCK_DGRAM */
> > +
> > +#ifndef SOCK_RAW
> > +#define SOCK_RAW 3
> > +#endif /* SOCK_RAW */
> > +
> > +#ifndef AF_LOCAL
> > +#define AF_LOCAL 1
> > +#endif /* AF_LOCAL */
> > +#ifndef AF_INET
> > +#define AF_INET 2
> > +#endif /* AF_INET */
> > +static inline int sys_socket(int domain, int type, int protocol)
> > +{
> > + return (ssize_t) syscall3(__SC_SOCKET,
> > + (long) domain,
> > + (long) type,
> > + (long) protocol);
> > +}
> > +
> > static inline int sys_exit(int status)
> > {
> > return (int) syscall1(__SC_EXIT,
> > diff --git a/plat/linuxu/include/linuxu/tap.h
> > b/plat/linuxu/include/linuxu/tap.h
> > index 65e6eb5..0f3a644 100644
> > --- a/plat/linuxu/include/linuxu/tap.h
> > +++ b/plat/linuxu/include/linuxu/tap.h
> > @@ -58,6 +58,13 @@ struct uk_in_addr {
> > uk_in_addr_t s_addr;
> > };
> >
> > +struct uk_sockaddr_in {
> > + uk_sa_family_t sin_family;
> > + uk_in_port_t sin_port;
> > + struct uk_in_addr sin_addr;
> > + __u8 sin_zero[8];
> > +};
> > +
> > struct uk_sockaddr {
> > uk_sa_family_t sa_family;
> > char sa_data[14];
> > diff --git a/plat/linuxu/tap_io.c b/plat/linuxu/tap_io.c
> > index 118bee8..888ef83 100644
> > --- a/plat/linuxu/tap_io.c
> > +++ b/plat/linuxu/tap_io.c
> > @@ -61,6 +61,37 @@ int tap_dev_configure(int fd, __u32
> > feature_flags,
> > void *arg)
> > return rc;
> > }
> >
> > +int tap_netif_configure(int fd, __u32 request, void *arg)
> > +{
> > + int rc;
> > + struct uk_ifreq *usr_ifr = (struct uk_ifreq *) arg;
> > +
> > + switch (request) {
> > + case UK_SIOCGIFHWADDR:
> > + case UK_SIOCSIFHWADDR:
> > + break;
> > + default:
> > + rc = -EINVAL;
> > + uk_pr_err("Invalid ioctl request\n");
> > + goto exit_error;
> > + }
> > +
> > + if ((rc = sys_ioctl(fd, request, usr_ifr)) < 0) {
> > + uk_pr_err("Failed to set device control %d\n",
> > rc);
> > + goto exit_error;
> > + }
> > +
> > + return 0;
> > +
> > +exit_error:
> > + return rc;
> > +}
> > +
> > +int tap_netif_create(void)
> > +{
> > + return sys_socket(AF_INET, SOCK_DGRAM, 0);
> > +}
> > +
> > int tap_close(int fd)
> > {
> > return sys_close(fd);
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |