lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 26 Jun 2010 13:16:12 +0200 From: Arnd Bergmann <arnd@...db.de> To: Chris Metcalf <cmetcalf@...era.com> Cc: linux-kernel@...r.kernel.org Subject: Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network. On Friday 25 June 2010, Chris Metcalf wrote: > The original submission of this code to LKML had the driver > instantiated under /proc/tile/hardwall. Now we just use a character > device for this, conventionally /dev/hardwall. Some futures planning > for the TILE-Gx chip suggests that we may want to have other types of > devices that share the general model of "bind a task to a cpu, then > 'activate' a file descriptor on a pseudo-device that gives access to > some hardware resource". As such, we are using a device rather > than, for example, a syscall, to set up and activate this code. The character device looks much better than the proc file. I still think a system call would be a more appropriate abstraction here, but the two are equivalent after all. > diff --git a/arch/tile/include/asm/processor.h b/arch/tile/include/asm/processor.h > index 96c50d2..95b54bc 100644 > --- a/arch/tile/include/asm/processor.h > +++ b/arch/tile/include/asm/processor.h > @@ -74,6 +74,14 @@ struct async_tlb { > unsigned long address; /* what address faulted? */ > }; > > +#ifdef CONFIG_HARDWALL > +struct hardwall_info; > + > +/* Can't use a normal list_head here due to header-file inclusion issues. */ > +struct hardwall_list { > + struct list_head *next, *prev; > +}; > +#endif It seems strange that you need this. Why does linux/list.h depend on asm/processor.h? It certainly seems that the code would get simpler if this can be avoided. > +/* > + * Guard changes to the hardwall data structures. > + * This could be finer grained (e.g. one lock for the list of hardwall > + * rectangles, then separate embedded locks for each one's list of tasks), > + * but there are subtle correctness issues when trying to start with > + * a task's "hardwall" pointer and lock the correct rectangle's embedded > + * lock in the presence of a simultaneous deactivation, so it seems > + * easier to have a single lock, given that none of these data > + * structures are touched very frequently during normal operation. > + */ > +static DEFINE_SPINLOCK(hardwall_lock); I think instead of making the lock more fine-grained, a better optimization might be to use RCU to protect the updates. AFAICT, the updates to the structure are rare but you need to read it a lot. > +/* > + * Low-level primitives > + */ > + > +/* Map a HARDWALL_FILE file to the matching hardwall info structure */ > +#define _file_to_hardwall(file) ((file)->private_data) > +#define file_to_hardwall(file) \ > + ((struct hardwall_info *)_file_to_hardwall(file)) I wouldn't bother with these abstractions, you can simply write file->private_data in every place where they are used. > +/* > + * Code to create, activate, deactivate, and destroy hardwall rectangles. > + */ > + > +/* Create a hardwall for the given rectangle */ > +static struct hardwall_info *hardwall_create( > + size_t size, const unsigned long __user *bits) > +{ To make this a system call, I'd wrap this function with one that roughly does asmlinkage long sys_hardwall_create(size_t size, const unsigned long __user *bits) { int fd; struct hardwall_info *info; info = hardwall_create(size, bits); if (IS_ERR(info)) return PTR_ERR(info); fd = anon_inode_getfd("hardwall", &hardwall_fops, info, O_CREAT | O_RDWR); if (fd < 0) hardwall_destroy(info); return fd; } The main difference between this and the ioctl approach would be that using chardev/ioctl allows you to give access to the hardwall functionality to a specific group or user with file permissions, while the system call would be less fine-grained, either associating access with a specific capability (e.g. CAP_SYS_ADMIN) or giving it to all users. > +#ifdef CONFIG_COMPAT > +static long hardwall_compat_ioctl(struct file *file, > + unsigned int a, unsigned long b) > +{ > + /* Sign-extend the argument so it can be used as a pointer. */ > + return hardwall_ioctl(file, a, (int)(long)b); > +} > +#endif Better use compat_ptr(b) instead of the manual cast. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists