|
[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 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?
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.
> + 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 |