From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12376 Path: news.gmane.org!.POSTED!not-for-mail From: Hauke Mehrtens Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v2] Add getrandom syscall wrapper and getentropy function Date: Sun, 21 Jan 2018 14:22:56 +0100 Message-ID: <64831ed9-fd78-281b-b522-fc581b0d4587@hauke-m.de> References: <20180106220809.10078-1-hauke@hauke-m.de> <20180109184813.GD1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Trace: blaine.gmane.org 1516540916 14299 195.159.176.226 (21 Jan 2018 13:21:56 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 21 Jan 2018 13:21:56 +0000 (UTC) To: musl@lists.openwall.com, Rich Felker Original-X-From: musl-return-12392-gllmg-musl=m.gmane.org@lists.openwall.com Sun Jan 21 14:21:51 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1edFYl-0001KS-RC for gllmg-musl@m.gmane.org; Sun, 21 Jan 2018 14:21:19 +0100 Original-Received: (qmail 23596 invoked by uid 550); 21 Jan 2018 13:23:14 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 23565 invoked from network); 21 Jan 2018 13:23:13 -0000 X-Virus-Scanned: amavisd-new at heinlein-support.de In-Reply-To: <20180109184813.GD1627@brightrain.aerifal.cx> Content-Language: en-US Xref: news.gmane.org gmane.linux.lib.musl.general:12376 Archived-At: On 01/09/2018 07:48 PM, Rich Felker wrote: > On Sat, Jan 06, 2018 at 11:08:09PM +0100, Hauke Mehrtens wrote: >> This syscall is available since Linux 3.17 and was also implemented in >> glibc in version 2.25 using the same interfaces. >> The getrandom function is a pure syscall wrapper liker glibc does it. >> getentropy is implemented on top of the getrandom syscall and fills the >> buffer. >> >> Currently no fallback is implemented this could be possible by using >> AT_RANDOM in the future. >> --- >> include/sys/random.h | 21 +++++++++++++++++++++ >> src/linux/getentropy.c | 29 +++++++++++++++++++++++++++++ >> src/linux/getrandom.c | 7 +++++++ >> 3 files changed, 57 insertions(+) >> create mode 100644 include/sys/random.h >> create mode 100644 src/linux/getentropy.c >> create mode 100644 src/linux/getrandom.c >> >> diff --git a/include/sys/random.h b/include/sys/random.h >> new file mode 100644 >> index 00000000..5b09e394 >> --- /dev/null >> +++ b/include/sys/random.h >> @@ -0,0 +1,21 @@ >> +#ifndef _SYS_RANDOM_H >> +#define _SYS_RANDOM_H >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#define __NEED_size_t >> +#define __NEED_ssize_t >> +#include >> + >> +#define GRND_NONBLOCK 0x0001 >> +#define GRND_RANDOM 0x0002 >> + >> +ssize_t getrandom(void *buf, size_t buflen, unsigned int flags); >> + >> +int getentropy(void *buffer, size_t length); > > Needs to drop namespace pollution in arg lists; just don't use names > here. Will do that. > >> +#ifdef __cplusplus >> +} >> +#endif >> +#endif >> diff --git a/src/linux/getentropy.c b/src/linux/getentropy.c >> new file mode 100644 >> index 00000000..48ca3d51 >> --- /dev/null >> +++ b/src/linux/getentropy.c >> @@ -0,0 +1,29 @@ >> +#include >> +#include >> +#include "syscall.h" >> + >> +int getentropy(void *buffer, size_t length) >> +{ >> + int ret; >> + char *pos = buffer; >> + size_t rem = length; >> + >> + if (length > 256) { >> + return __syscall_ret(-EIO); >> + } >> + >> + while (rem) { >> + ret = __syscall_cp(SYS_getrandom, pos, rem, 0); > > My understanding was that the glibc choice was not to make getentropy > a cancellation point. If that's correct, is there a reason you want to > do it differently? Ok I will change this, glibc made getentropy no cancellation point, but getrandom is a cancellation point I will do it the same way here. >From glibc documentation: > The @code{getentropy} function is not a cancellation point. A call to > @code{getentropy} can block if the system has just booted and the > kernel entropy pool has not yet been initialized. In this case, the > function will keep blocking even if a signal arrives, and return only > after the entropy pool has been initialized. > The @code{getrandom} function is a cancellation point. Source: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225 > >> + if (ret == -EINTR) { >> + continue; > > This in particular seems inconsistent with making it cancellable, but > maybe the idea is to avoid issues where the caller does not check the > return. > >> + } else if (ret == -EFAULT || ret == -ENOSYS) { >> + return __syscall_ret(ret); >> + } else if (ret <= 0) { >> + return __syscall_ret(-EIO); >> + } > > Is there a reason for collapsing some possible error conditions but > not others? The man page said that getentropy only returns, EFAULT, EIO or ENOSYS. Should I forward all errors? >> + >> + pos += ret; >> + rem -= ret; >> + } >> + return 0; >> +} > > Overall, my leaning for getentropy would be to implement it as a > wrapper around getrandom using the public interfaces (e.g. > pthread_setcancelstate if cancellation is to be blocked, and errno > rather than -EFOO stuff), with the idea that it should not need to be > changed if we later implement fallbacks for getrandom and that it be a > "generic" file that not depend on musl internals. In glibc it is not a cancellation point, so should I call the syscall directly with no cancellation point or should I call the getrandom()) function which has a cancellation point? >> diff --git a/src/linux/getrandom.c b/src/linux/getrandom.c >> new file mode 100644 >> index 00000000..795b932c >> --- /dev/null >> +++ b/src/linux/getrandom.c >> @@ -0,0 +1,7 @@ >> +#include >> +#include "syscall.h" >> + >> +ssize_t getrandom(void *buf, size_t buflen, unsigned int flags) >> +{ >> + return syscall_cp(SYS_getrandom, buf, buflen, flags); >> +} > > This part looks fine, I think. > > Rich > Hauke