zlua is a maintenance problem with low bang/buck as implemented...?

[adamdmoss]

I admire and enjoy Lua.
I don't know what it's really doing in ZFS though (channel programs? wha?), and least of all why we need to put it in the darn kernel if it's not twiddling hardware bits or deeply coupled to kernel structures. It's a frickin' interpreter for a scripting language to support one feature, and as small and elegant as Lua is it hurts my head to think of it in ring-zero.
So while I admit my caveman brain is having a knee-jerk reaction to something I don't fully understand, nonetheless zlua is 15KLloc of code which:

• is heavily modified from upstream in ways that create unique issues and make it harder to accept upstream fixes
• is a constant source of bugs and holes (see: git log)
• does not appear owned or maintained short of when users or code analyzers tell us it's full of bugs and holes
• surely should live in userspace if it should live at all

[ryao]

@adamdmoss It made certain things faster by eliminating syscall overhead.

That said, I was worried about lua being a source of bugs since the beginning, but it is behind the SYS_CONFIG privilege. There are plenty of things that someone with that privilege could do to try to compromise a system, so the risk is mostly mitigated by that.

A quick check finds that the imported lua code is vulnerable to GHSA-v3hh-4h88-w4mr (which is unfortunate considering that the patch was available when the lua code was merged). All of the existing CVEs probably need to be checked to see if they apply:

https://www.opencve.io/cve?vendor=lua

A LUA update might be a good idea too, but I am not very familiar with the LUA code, so I am taking things one step at a time.

I do not feel strongly about keeping/ejecting LUA (since putting it behind SYS_CONFIG was enough to dissuade me from voicing objections when it was first proposed), but I suspect that the guys at Delphix would feel strongly against ejecting it (ping @ahrens).

I am slowly working my way through all of the coverity reports. My current plan is to keep patching things until static analyzers stop telling us that there are problems (and there are no more bug reports). That is actually my current plan for the entire codebase, so there is nothing about it that is unique to the lua code.

[ryao]

It occurs to me that if people start sharing lua scripts for ZCP, then it would be possible for someone to share a malicious script that would exploit a LUA bug. This makes getting the LUA interpreter correct very important.

[ryao]

In hindsight, I am curious why Lua was used rather than the D interpreter from DTrace. DTrace’s D interpreter was designed for use inside the kernel and is presumably less buggy.

[rincebrain]

Given that when Oracle landed DTrace in the Linux kernel, it had (and perhaps still has? I haven't checked, and I thought they ported that to using an eBPF backend anyway) a problem with regularly panicking, perhaps that was limited to how well it was integrated in Solaris.

[ghost]

Something I've been wondering about lately is why libzfs does zcp_check() to check that channel programs see the same property values as whatever ioctl the value came from, just so it can print a warning when they differ.

It's also been on my mind recently that our Lua supports only int64_t numbers, so any property that has a uint64_t numeric value has to be explicitly cast to the right type after pulling it out of the results nvlist. This is easily noticeable when using the json ouput of zfs program -j, for example.

I think channel programs are a pretty neat idea, being able to construct complex operations in a simple language and even use them with a single ioctl from userspace, but there still is not a whole lot you can actually do with them in practice. Try to implement zfs list (just that, no other options/args) as a channel program - you can't, channel programs can't iterate over pools. You have to give one pool it's allowed to know about.

It was interesting to see #13802 come through recently. Now you can rename a snapshot in a channel program. You still can't rename a dataset though.

I hope to eventually see channel programs be capable of more. The less I have to rely on libzfs the better. Channel programs give you one ioctl that has the potential to be incredibly flexible and powerful. But it is true that right now it's a pretty big chunk of code that doesn't do much.

[stale]

This issue has been automatically marked as "stale" because it has not had any activity for a while. It will be closed in 90 days if no further activity occurs. Thank you for your contributions.

[darkpixel]

As a Python developer who stumbled upon Lua in ZFS while trying to get ZFS data into JSON format...all I can say is I hate Lua with a passion. I've been banging around for about 6 hours now trying to get some pretty simple stuff out of ZFS, but I had to learn that Lua can't do print(somevar) in ZFS because I guess it's disabled. I'm sure I've screwed something up while trying to recursively descend through all the datasets in a pool...but it's damned near impossible to debug, not having support for "arrays" is atrocious, and the best I can do is spit out a bunch of JSON dictionaries when some things should be lists. And strangely enough, there's only "zfs commands"...i.e. taking/destroying snapshots, creating/deleting datasets, etc...but nothing zpool-related. Like no ability to get pool status, start/stop a scrub or trim, etc...

I think it's a shame that JSON output for the zfs and zpool commands was dumped in favor of adding this horrific abomination of a programming language where you have to upload a script to a machine, then execute zpool program against the script.

zfs list --json -o space,compression,compressratio,encryption,keystatus would have been about a million times easier to deal with.

[robn]

@darkpixel the upcoming OpenZFS 2.3 series has JSON support and will do what you suggest, see #16217.

(the rest is largely irrelevant; whatever the channel program facility is, it is not really anything to do with getting information out of the system, and it certainly wasn't implemented "in favour of" support for JSON command output).

[darkpixel]

Thanks @robn. That's great news.
I just decided to give up for the night:

~ $ sudo zfs program -j -n ai test.zcp tank | jq .
Channel program execution failed:
Memory limit exhausted.
~ $ 

That's just walking all the datasets under tank and grabbing the snapshots. If I do something slightly smaller like recursively grabbing properties from all the datasets it works perfectly. I'm sure there's a knob somewhere that sets how much memory the lua script gets, but I'll just patiently wait for JSON support.

[robn]

zfs-program(8) documents the knob: -m memory-limit. But yeah, this is really not what channel programs are for at all, so it's not surprising you're struggling with it.

[darkpixel]

Oops--I totally missed that in the man page.

So what are channel programs for then? The man page shows they basically get/set datasets, properties, snapshots, bookmarks, etc...

[robn]

So what are channel programs for then? The man page shows they basically get/set datasets, properties, snapshots, bookmarks, etc...

@darkpixel the idea is to be able run complex, bulk and/or multi-step administrative processes "atomically" by loading a program into ZFS itself, which it can then run as a group, under a single lock, without other administrative operations getting in the way.

A really contrived example might be to destroy and then recreate a dataset. In a shell script, that would be zfs destroy ... ; zfs create ... but something can come along after the destroy and take that dataset name or make some other change that would stop the create from working, leaving things in an in-between state. In a channel program, that can't happen.

I'm deliberately not speaking to the general usefulness, implementation details, etc, because this issue is not really the right place for that. Suffice to say, there's lots of different ways this could be done; loading Lua scripts into the ZFS module is the one we have right now.

[darkpixel]

Thanks @robn. Sounds like it's just a handy way of performing multiple steps as a single "transaction".

[robn]

@darkpixel perfect summary 👍