I assume I'm missing something in the linux syscall implementation, but where is this version implemented?

It looks like there might be a whitespace consistency issue yet.

P.S. Not to be fixed in this commit, but this should really be using designated initializers (e.g. [LINUX_SENDFILE] = 4).

I left this out to be corrected. I tried this and another version where I tried referencing linux_sendfile for sendfile64 but I can't seem to get it to work. It always crashes. I am open to any direction on how to properly implement the 64 bit version for 32 bit linux. There also shouldn't be any difference between the implementations, hence I tried just referencing linux_sendfile.

There are tabs before the second column in all instances. It looks fine using diff tools and on vim. This might just be phabricator not showing it correctly, unless my editor is doing something weird.

int error prefered

missied fp, you need fdrop()

We don't seem to use the result of this call anywhere. I think it is unnecessary.

I will remove this call

It seems we don't actually write to the offset?

this cast isn't right - we need something like:

l_loff_t offset;
int rv;

offset = arg->offset;
rv = linux_sendfile_common(..., &offset, ...);
// do something with offset
return (rv);

In Linux, sendfile() may also adjust the file's seek offset: "If offset is not NULL, then sendfile() does not modify the file offset of in_fd; otherwise the file offset is adjusted to reflect the number of bytes read from in_fd." In particular, if offset == NULL there is still some work that you need to do.

As a hint for how this should be fixed, read the manual page for the lseek() system call, which does nothing but adjust the seek offset for a file descriptor. Also take a look at its implementation in FreeBSD, in kern_lseek().

The way we are passing arg->offset (l_long) here is not right.

Suppose you are running a 32-bit Linux application on a 64-bit amd64 kernel. From the application's point of view, sizeof(offset) is 4 but from the kernel's point of view sizeof(offset) is 8. When linux_sendfile_common() uses copyout() to copy the returned offset back into userspace, it will always copy 8 bytes. 32-bit applications, which expect only 4 bytes, are going to experience memory corruption as a result.

arg->offset is l_loff_t*

it looks like Linux's definition is sendfile64(..., __off64_t *offset, ...)

Sorry, I put the comment in the wrong place. The issue is with linux_sendfile(), where arg->offset is l_long_t *.

l_loff_t * is equivalent. It would probably be clearer to define l_off64_t and use that instead, but l_loff_t is what we use elsewhere in this file.

ended up with some thing:

int
linux_sendfile(struct thread *td, struct linux_sendfile_args *arg)
{
	if (arg->offset == NULL)
		return linux_sendfile_common(td, arg->out, arg->in,
			NULL, arg->count);

	l_loff_t offset64;
	int ret;
	int error;
	l_long offset;

	error = copyin(arg->offset, &offset, sizeof(offset));
	if (error < 0)
		return (error);
	
	offset64 = (l_loff_t) offset;

	ret = linux_sendfile_common(td, arg->out, arg->in,
		&offset64, arg->count);

	offset = (l_long) offset64;
	error = copyout(&offset, arg->offset, sizeof(offset));
	if (error < 0)
		return (error);
	
	return (ret);
}

I will clarify the part according to mark's comment below

kern_lseek() returns an error number or 0, not the offset.

It is an unusual interface in that it returns the offset by setting a field in the current thread's structure. Take a look at vn_seek() or shm_seek(), which implement the fo_seek() interface: the set td->td_uretoff.tdu_off to the offset, where td is the current thread (curthread, a magic global variable). You need to fetch the seek offset from there.

(In my opinion fo_seek() should return the offset in an output argument and kern_lseek() should set curthread->td_uretoff.tdu_off, but this is a separate discussion.)

Instead of using kern_lseek(), it is better to call fo_seek() directly, before fdrop()ing the file. Otherwise this function is doing three fget()s per call, which is unnecessary, and moreover it introduces some ugly error cases. For instance, suppose the process closes the fd after the fdrop() call but before the second kern_lseek() call. Then the second kern_lseek() call will fail, so sendfile() will return an error (EBADF) even though it technically succeeded. That is not a fatal problem, but it is better to avoid it.

In fact, using fo_seek() itself is a bit racy. Right now we have the following sequence of operations when offset == NULL:

1. load current offset
2. call fo_sendfile() with the current offset
3. update the current offset

Suppose another thread changes the current offset between 1 and 3: the result of the call isn't really consistent. If you look at the fo_seek() implementations, like vn_seek(), you will see that foffset_lock() and foffset_unlock() are used to synchronize operations on the file's current offset. I think it is ok to ignore this problem, I am just mentioning it for completeness.

As a matter of style I think this function and the one below should contain only a single call to linux_sendfile_common(). That is, write:

if (offset != NULL)
    <copyin>
ret = linux_sendfile_common()
if (offset != NULL)
    <copyout>

At least, per FreeBSD style, local variable declarations always come before code.

Should I not use kern_lseek here as well and go with fo_seek?

Yes, I think you can fetch the current offset after the fget_read() call, using fo_seek().

The indentation looks wrong here, like there's a mix of tabs and spaces being used.

fo_sendfile() returns a positive error number (see sys/sys/errno.h).

Style: there should be no space after the cast.

Style: there should be no space after the cast.